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

Use hardware performance counter data for the detailed/self-profile data view #1345

Open
wesleywiser opened this issue Jun 15, 2022 · 11 comments · Fixed by #1647
Open

Use hardware performance counter data for the detailed/self-profile data view #1345

wesleywiser opened this issue Jun 15, 2022 · 11 comments · Fixed by #1647
Labels
A-collector Issues related to the collector

Comments

@wesleywiser
Copy link
Member

Now that rustc supports using HPC data in -Zself-profile (rust-lang/rust#78781), it would be great to use this support on perf.rlo as well. Many of our smaller benchmarks don't run long enough for the std::time::Instant based profiling to work reliably which makes it hard to interpret the data when it doesn't really match the results reported on the summary page for a particular benchmark. By using the HPC data, hopefully this will improve the accuracy of detailed data view.

@wesleywiser wesleywiser added the A-collector Issues related to the collector label Jun 15, 2022
@eddyb
Copy link
Member

eddyb commented Jun 17, 2022

instructions:u is also the default for the total counts, so it seems natural to compare it, instead of time - often there is a meaningful change in the total instructions:u, but it's lost in time noise in the query view.

Also, assuming it (still) works, I recommend -Z self-profile-counter=instructions-minus-irqs:u (instead of the plain instructions:u), to avoid weird jitter in otherwise-deterministic queries.

@eddyb
Copy link
Member

eddyb commented Jun 24, 2022

Oh dear, it won't work yet, it's broken without adding features = ["nightly"] or updating the version of measureme that rustc uses (see rust-lang/rust#78781 (comment)).

@wesleywiser
Copy link
Member Author

Opened rust-lang/rust#98471 to update measureme in rustc to resolve that.

@klensy
Copy link
Contributor

klensy commented Jul 23, 2022

Opened rust-lang/rust#98471 to update measureme in rustc to resolve that.

Merged.

@Kobzol
Copy link
Contributor

Kobzol commented Jul 7, 2023

I have tried to implement this, but I don't know how to actually read the HW counter data from the output of -Zself-profile. I'm using -Zself-profile -Zself-profile-counter=instructions:u, but the output file, when processed with summarize summarize --json, only contains information about time (I don't see any counter values).

@bjorn3
Copy link
Member

bjorn3 commented Jul 7, 2023

I think profiles either contain timestamps or instructions:u values, not both. Did you check that summarize summarize --json outputs values that make sense if the counters are time rather than instructions?

@Kobzol
Copy link
Contributor

Kobzol commented Jul 8, 2023

To be honest, I'm not really sure how to recognize that. It's true that when I turn on HW counters, the times seem to be diferent by orders of magnitude. So the counter values just get stored in the nanos attribute of time?

@lqd
Copy link
Member

lqd commented Jul 8, 2023

The PR introducing the feature in rustc did mention this as somewhat backwards compatible for tools, until they adapt to the new counters. Maybe summarize is in that category.

@Kobzol
Copy link
Contributor

Kobzol commented Jul 8, 2023

Yeah I saw that, but somehow I expected that this adoption has already happened in these 3 years 😅 Maybe not, I'll check how the tools work.

@Kobzol
Copy link
Contributor

Kobzol commented Jul 8, 2023

Yeah it seems to just output time as nanoseconds.

@Kobzol
Copy link
Contributor

Kobzol commented Aug 12, 2023

Reopening, because #1647 had to be reverted because of some issue with measureme.

@Kobzol Kobzol reopened this Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collector Issues related to the collector
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants