self-profile: Cache more query key strings when doing self-profiling. #75452
Conversation
|
r? @lcnr (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
cc @matthewjasper on the specialization of |
| // "WithOptConstParam { did: foo::bar, const_param_did: Some(foo::baz) }" | ||
| // becomes "(foo::bar, foo::baz)" and | ||
| // "WithOptConstParam { did: foo::bar, const_param_did: None }" | ||
| // becomes "(foo::bar, _)". |
There was a problem hiding this comment.
What does self profiling need the WithOptConstParam for?
You probably want to uniquely identify query calls? Considering that self.const_param_did should be unique for each DefId (as we otherwise cause an ICE while building mir) you might also be able to store this as (did, bool) for some additional wins.
There was a problem hiding this comment.
What does self profiling need the
WithOptConstParamfor?
Some queries (e.g. mir_build) take WithOptConstParam values as keys. Since self-profiling can record a string representation for each query key that gets used some of those keys will be stringified WithOptConstParam values. You can find the full list of queries (and their keys) in https://github.com/rust-lang/rust/blob/master/src/librustc_middle/query/mod.rs.
You probably want to uniquely identify query calls? Considering that
self.const_param_didshould be unique for eachDefId(as we otherwise cause an ICE while building mir) you might also be able to store this as(did, bool)for some additional wins.
I'm not quite sure I follow. You are saying that there is an invariant around const_param_did that makes sure that it is always the same (or None) for a given did value?
In any case, I think it's a good idea to keep this as close to the original value as possible. The encoding format should make sure that there is no data redundancy (assuming that that DefId was already mentioned somewhere already).
There was a problem hiding this comment.
Since self-profiling can record a string representation for each query key that gets used
That's what I wasn't sure about, thanks 👍
I'm not quite sure I follow. You are saying that there is an invariant around const_param_did that makes sure that it is always the same (or None) for a given did value?
yes.
In any case, I think it's a good idea to keep this as close to the original value as possible. The encoding format should make sure that there is no data redundancy (assuming that that DefId was already mentioned somewhere already).
👍
| // "WithOptConstParam { did: foo::bar, const_param_did: None }" | ||
| // becomes "(foo::bar, _)". | ||
|
|
||
| let mut components: SmallVec<[StringComponent<'_>; 8]> = SmallVec::new(); |
There was a problem hiding this comment.
I believe this can be a SmallVec<[StringComponent<'_>; 5]>
There was a problem hiding this comment.
do we even need a smallvec? Wouldn't a simple array work here? ([StringComponent<'_>; 5])
There was a problem hiding this comment.
Ah yes, that's true. I'll change that.
michaelwoerister
force-pushed
the
sp-cache-more-query-keys
branch
from
b808e98
to
08d9517
Compare
|
while I am not too familiar with @bors r+ |
|
📌 Commit 08d9517 has been approved by |
bors
added
S-waiting-on-bors
|
Thanks for the review! |
|
We don't profile in verbose mode in perf.rlo so this shouldn't have any effect on our measurements. @bors rollup |
Rollup of 12 pull requests Successful merges: - rust-lang#74650 (Correctly parse `{} && false` in tail expression) - rust-lang#75319 (Fix ICE rust-lang#75307 in `format`) - rust-lang#75417 (Don't spill operands onto the stack in naked functions) - rust-lang#75452 (self-profile: Cache more query key strings when doing self-profiling.) - rust-lang#75459 (fix LocalInfo doc comment) - rust-lang#75462 (Remove unused tcx parameter) - rust-lang#75467 (Fix E0741 error code explanation) - rust-lang#75471 (Change registered "program name" for -Cllvm-args usage messages) - rust-lang#75477 (Expand function pointer docs) - rust-lang#75479 (make rustc-docs component available to rustup) - rust-lang#75496 (Prioritization WG: Open Zulip topics only for `I-prioritize` issues) - rust-lang#75500 (Disable zlib in LLVM on aarch64-apple-darwin) Failed merges: r? @ghost
This PR adds optimized
SpecIntoSelfProfilingStringimplementations for two common query key types (LocalDefIdandWithOptConstParam). This makes raw self-profiling data on disk 8-9% smaller for my two test cases (regexandcargo).The on-disk format is not affected, so no tooling updates need to happen.
I also tried adding an impl for
Ty<'tcx>(which should reduce size quite a bit) but the compiler did not allow me to add a specialized impl parameterized with'tcx. I don't know if there is an actual problem with that or if the implementation of specialization just doesn't support it yet.cc @wesleywiser @Mark-Simulacrum