Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add performance analysis tool #167

Merged
merged 1 commit into from
Nov 19, 2024
Merged

Add performance analysis tool #167

merged 1 commit into from
Nov 19, 2024

Conversation

michaelmior
Copy link
Collaborator

No description provided.

contrib/CMakeLists.txt Outdated Show resolved Hide resolved
contrib/perf.cc Outdated
static long perf_event_open(struct perf_event_attr *hw_event, pid_t pid,
int cpu, long group_fd, unsigned long flags) {
long fd;
fd = syscall(SYS_perf_event_open, hw_event, pid, cpu, group_fd, flags);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should not test that hw_event is not empty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only called within this one file and guaranteed to be initialized correctly, so I don't think this is a concern.

contrib/perf.cc Outdated Show resolved Hide resolved
contrib/perf.cc Outdated Show resolved Hide resolved
contrib/perf.cc Show resolved Hide resolved
@tony-go
Copy link

tony-go commented Nov 19, 2024

Also as a reader/user I would like to have a readme with a few explanations, a few snippets.

Ideally we would like to have a run job on the CI just to illustrate it :)

Great job @michaelmior 😍

@michaelmior
Copy link
Collaborator Author

Any idea why CI is failing on Linux? (I'd expect this for macOS/Windows.)

@michaelmior michaelmior force-pushed the perf branch 2 times, most recently from 4cce79a to 84a6540 Compare November 19, 2024 16:22
@michaelmior
Copy link
Collaborator Author

@tony-go Fair enough with the suggestion of examples. I didn't see this with any existing tools which is why I didn't add anything. Any suggestion where such docs should live?

@jviotti
Copy link
Member

jviotti commented Nov 19, 2024

Yeah, I would deal with README + CI runs for these kind of things in a separate PR if any, as we are not doing it for any of them.

contrib/CMakeLists.txt Outdated Show resolved Hide resolved
@michaelmior michaelmior force-pushed the perf branch 3 times, most recently from 72be042 to b20008d Compare November 19, 2024 18:32
Signed-off-by: Michael Mior <[email protected]>
@michaelmior
Copy link
Collaborator Author

michaelmior commented Nov 19, 2024

@jviotti There were a couple formatting issues I corrected, but now all the CI failures are due to Error: Resource not accessible by integration.

@jviotti
Copy link
Member

jviotti commented Nov 19, 2024

Ah, it's the benchmark comments. I guess they can't post if they are coming from PRs 😓 Let me merge as-is and I'll try to fix it later

@jviotti jviotti merged commit 16b8531 into sourcemeta:main Nov 19, 2024
9 of 14 checks passed
@jviotti
Copy link
Member

jviotti commented Nov 19, 2024

I made an upstream issue here: benchmark-action/github-action-benchmark#279. Maybe we are missing some setting or something.

@michaelmior
Copy link
Collaborator Author

Strange! I would have said that would be because you need to manually specify a GitHub token, but you're already doing that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants