-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Collect statistics about MIR optimizations #76769
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
use std::io::{self, Write as _}; | ||
use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; | ||
|
||
static COLLECTOR: Collector = Collector::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be part of Session
or TyCtxt
. There may be multiple rustc sessions running in a single process. (for example when using RLS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I experimented with different implementation approaches:
- Statistics definitions are in the context together with their values.
- Statistics definitions are in MIR passes, their values are stored in the context.
- Statistics definitions and values are stored together with MIR passes.
I strongly prefer variants that put statistics definitions together with MIR passes that uses them. In that approach, adding and removing counters is trivial, additionally any unused counters are detected as such during compilation.
Values can be stored in the context if we want to support multiple rustc sessions in a single process, although it requires passing context to all those places where counters are used. But is there any real use-case? Why would you use rustc with debug-assertions and MIR optimizations counters inside the RLS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @bjorn3. We've tried pretty hard to move these sorts of things into Session
and TyCtxt
to allow running multiple sessions in the same process (both for RLS and eventually for one rustc process compiling multiple crates simultaneously).
Since this is essentially unstable, diagnostic data and doesn't effect the compilation artifacts, it may be worth bending that rule because, as you point out, having the counters defined in their MIR passes is quite nice. I don't personally feel comfortable approving that since this is a cross-cutting concern for the compiler as a whole and there may be others on the compiler team that have strong opinions about breaking that rule.
r? @wesleywiser |
b0a81f4
to
9996642
Compare
☔ The latest upstream changes (presumably #76659) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:
|
63abbc7
to
511d534
Compare
☔ The latest upstream changes (presumably #77430) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:
|
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author |
b5d9534
to
a2293bf
Compare
☔ The latest upstream changes (presumably #77796) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:
|
a2293bf
to
c833057
Compare
☔ The latest upstream changes (presumably #77373) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:
|
For example -Zmir-opt-level=3 -Zmir-opt-stats: