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

Include the profile in a result's summary; more documentation for rust-timer commands #1146

Merged
merged 2 commits into from
Jan 17, 2022

Conversation

lqd
Copy link
Member

@lqd lqd commented Jan 13, 2022

Fixes #1048 by changing the summary line of a benchmark result by adding its profile as well.

This matches the compare page's "Benchmark & Profile" column, so I think it's clearer in that sense (but maybe it can be put elsewhere in the summary sentence). But this change will also impact the automated weekly report: I therefore wonder if this is how you'd want it to be fixed ?

For example, this summary would now mention that the regression was in a doc benchmark: from "Very large regression in instruction counts (up to 6.6% on full builds of regression-31157)" to "Very large regression in instruction counts (up to 6.6% on full builds of regression-31157 doc)".

And this one would go from "Large regression in instruction counts (up to 1.6% on incr-unchanged builds of derive)" to "Large regression in instruction counts (up to 1.6% on incr-unchanged builds of derive check)"

Resolves #1099 by adding a tiny bit more documentation about rust-timer commands on the Help page, and the link Scott suggests.

Here's how it looks rendered

image

Copy link
Member

@rylev rylev left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I'd like to see @Mark-Simulacrum's opinion especially on the changes to the help page.

@Mark-Simulacrum
Copy link
Member

Seems good to me. I think we may wish to consider linking the scenario and profile to documentation (e.g., https://github.com/rust-lang/rustc-perf/blob/master/docs/glossary.md) and the benchmark name to the relevant line in https://github.com/rust-lang/rustc-perf/blob/master/collector/benchmarks/README.md -- but that's a larger project.

FWIW, in the future, it seems nice to split this PR into two (for each part of the changes).

@Mark-Simulacrum Mark-Simulacrum merged commit a4fbd42 into rust-lang:master Jan 17, 2022
@lqd lqd deleted the profile_summary branch January 17, 2022 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants