-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[perf] GenericArgs
-related: Change asserts to debug asserts & use more slice interning over iterable interning
#142289
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
Conversation
c9e7983
to
e2e88ca
Compare
e2e88ca
to
5ed5135
Compare
GenericArgs::truncate_to
and AliasTerm::own_args
GenericArgs
-related functions
@bors2 try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
[perf] Tweak `GenericArgs`-related functions
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (c7e6acc): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -9.0%, secondary 2.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -1.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 753.145s -> 754.777s (0.22%) |
196f52a
to
642bb6d
Compare
I want to check what happens if I only do the @bors2 try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
[perf] Tweak `GenericArgs`-related functions Result: #142289 (comment)
Wait, I want to set parent=last @bors2 try cancel |
Try build cancelled. Cancelled workflows: |
@bors2 try parent=40daf23eeb711dadf140b2536e67e3ff4c999196 @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
[perf] Tweak `GenericArgs`-related functions Result: #142289 (comment)
e5e876a
to
3a31f62
Compare
GenericArgs
-related functionstrait_ref_own_args
: Change assert to debug assert and use more slice interning over iterable interning
let method_args = | ||
tcx.mk_args_from_iter(trait_args.iter().skip(callee_generics.parent_count)); |
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.
Just a drive-by change, definitely not relevant or hot.
debug_assert_matches!(self.def_kind(def_id), DefKind::AssocTy | DefKind::AssocConst); | ||
let trait_def_id = self.parent(def_id); | ||
assert_matches!(self.def_kind(trait_def_id), DefKind::Trait); | ||
debug_assert_matches!(self.def_kind(trait_def_id), DefKind::Trait); |
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.
The main and most relevant positive perf impact comes from changing these asserts to debug ones. Results: #142289 (comment).
@@ -588,7 +588,7 @@ impl<'tcx> GenericArgs<'tcx> { | |||
} | |||
|
|||
pub fn truncate_to(&self, tcx: TyCtxt<'tcx>, generics: &ty::Generics) -> GenericArgsRef<'tcx> { | |||
tcx.mk_args_from_iter(self.iter().take(generics.count())) | |||
tcx.mk_args(&self[..generics.count()]) |
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 one wasn't tested in isolation (three perf runs are already a lot (I had to narrow down what is and what isn't perf relevant)) but I presume it to be helping since slicing & slice interning should generally be faster than iterating & "iterable interning" and this function is semi hot / lukewarm I'm pretty sure.
The first approach (which contains this change) yielded more improvements (results: #142289 (comment) (e.g., for image
)) than the 1st commit (the assert changes) (results: #142289 (comment)), so this one might be the cause?
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 could rerun perf for this change / commit only (which seems a bit overkill) or just drop it entirely? WDYT?
r? compiler-errors (since you've reacted in this thread) or lcnr or compiler |
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.
cool. i dont think we'll gain much insight by doing individual perf tests of all the tweaks here; the fact that we gain a bit of perf back anyways is good and the changes at least make the code more readable.
r=me when ci is green or sooner i dont care much |
trait_ref_own_args
: Change assert to debug assert and use more slice interning over iterable interningtrait_ref_own_args
: Change asserts to debug asserts and use more slice interning over iterable interning
trait_ref_own_args
: Change asserts to debug asserts and use more slice interning over iterable interningGenericArgs
-related: Change asserts to debug asserts & use more slice interning over iterable interning
@bors r=compiler-errors |
@bors p=5 (to get some |
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing cc87afd (parent) -> 49a8ba0 (this PR) Test differencesNo test diffs found Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 49a8ba06848fa8f282fe9055b4178350970bb0ce --output-dir test-dashboard And then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (49a8ba0): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary -2.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 756.218s -> 758.463s (0.30%) |
[perf] `GenericArgs`-related: Change asserts to debug asserts & use more slice interning over iterable interning 1. The 1st commit yields the following perf gains: [#142289 (comment)](rust-lang/rust#142289 (comment)). 2. The 2nd commit might also have a minor positive perf impact, however that one wasn't tested in isolation. For reference, the initial approach rust-lang/rust@c7e6acc (results: rust-lang/rust#142289 (comment)) had a lot more changes (apart from what's now contained in commit 1 and 2) which seemed to be perf irrelevant (cf. the partial countercheck in rust-lang/rust@6f82bf1 (results: rust-lang/rust#142289 (comment)).
[perf] `GenericArgs`-related: Change asserts to debug asserts & use more slice interning over iterable interning 1. The 1st commit yields the following perf gains: [#142289 (comment)](rust-lang/rust#142289 (comment)). 2. The 2nd commit might also have a minor positive perf impact, however that one wasn't tested in isolation. For reference, the initial approach rust-lang/rust@c7e6acc (results: rust-lang/rust#142289 (comment)) had a lot more changes (apart from what's now contained in commit 1 and 2) which seemed to be perf irrelevant (cf. the partial countercheck in rust-lang/rust@6f82bf1 (results: rust-lang/rust#142289 (comment)).
For reference, the initial approach c7e6acc (results: #142289 (comment)) had a lot more changes (apart from what's now contained in commit 1 and 2) which seemed to be perf irrelevant (cf. the partial countercheck in 6f82bf1 (results: #142289 (comment)).