Skip to content

Overhaul how the rustc benchmark works. #1140

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

Merged

Conversation

nnethercote
Copy link
Contributor

Currently the rustc benchmark is handled like a normal benchmark in
some ways, and handled very differently in others. This results in
various non-obvious special cases.

This commit improves things.

  • Adds a --bench-rustc option to bench_local and bench_next, which
    makes the rustc benchmark opt-in.
  • Omits the rustc benchmark from --include/--exclude handling.
  • Calls it the "special rustc benchmark", in contrast to the "normal
    benchmarks".
  • Handles it directly in bench(), avoiding a bunch of awkward
    special-casing code.
  • Adds an eprintln! to make it more obvious when it starts running.
  • Cleans up some unnecessary string conversions in bench().

Because it's created in the `bench()` function which is called for each
of the `bench*` subcommands.
@adamgemmell
Copy link
Contributor

This seems like a good time to add a note in the README about how the artifact ID will be used as the rust-lang/rust ref to benchmark when using bench_local --bench-rustc. I don't believe it's mentioned anywhere in the documentation otherwise and it's quite important to know

@Mark-Simulacrum
Copy link
Member

This looks largely good to me -- the note @adamgemmell suggests seems like a good idea.

This likely should also update https://github.com/rust-lang/rustc-perf/blob/master/collector/collect.sh#L29 to make sure we continue collecting rustc on the perf.rlo collector.

@nnethercote
Copy link
Contributor Author

@nnethercote nnethercote force-pushed the overhaul-rustc-benchmark branch from 0ab786c to 4c716d4 Compare January 10, 2022 21:46
@nnethercote
Copy link
Contributor Author

I have fixed the formatting and added the note about the ID in the second commit.

Currently the `rustc` benchmark is handled like a normal benchmark in
some ways, and handled very differently in others. This results in
various non-obvious special cases.

This commit improves things.
- Adds a `--bench-rustc` option to `bench_local` and `bench_next`, which
  makes the rustc benchmark opt-in.
- Omits the rustc benchmark from `--include`/`--exclude` handling.
- Calls it the "special rustc benchmark", in contrast to the "normal
  benchmarks".
- Handles it directly in `bench()`, avoiding a bunch of awkward
  special-casing code.
- Adds an `eprintln!` to make it more obvious when it starts running.
- Cleans up some unnecessary string conversions in `bench()`.
Also import more identifiers from `execute`, for consistency.
@nnethercote nnethercote force-pushed the overhaul-rustc-benchmark branch from 4c716d4 to d013f6d Compare January 11, 2022 00:10
@Mark-Simulacrum Mark-Simulacrum merged commit 81ddb42 into rust-lang:master Jan 11, 2022
@nnethercote nnethercote deleted the overhaul-rustc-benchmark branch January 11, 2022 02:37
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