-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Ergonomic ref counting: optimize away clones when possible #139088
base: master
Are you sure you want to change the base?
Conversation
let mir = instance.instantiate_mir_and_normalize_erasing_regions( | ||
tcx, | ||
ty::TypingEnv::fully_monomorphized(), | ||
ty::EarlyBinder::bind(mir.clone()), |
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.
We need to clone the mir body here, unsure how bad this could be.
This comment has been minimized.
This comment has been minimized.
5592732
to
961717a
Compare
This comment has been minimized.
This comment has been minimized.
961717a
to
07cc495
Compare
This comment has been minimized.
This comment has been minimized.
07cc495
to
28551ae
Compare
This comment has been minimized.
This comment has been minimized.
28551ae
to
792d2ce
Compare
This comment has been minimized.
This comment has been minimized.
@@ -1,52 +0,0 @@ | |||
//@ known-bug: #129372 |
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.
Would need to properly decide exactly what to do with this test. If just removing it is fine or if there's some variation that needs to be placed in tests/ui
. Didn't bother to check this for now.
This comment has been minimized.
This comment has been minimized.
0a7a642
to
f6c3c4b
Compare
This PR changes a file inside This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @vakaras Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
This comment has been minimized.
This comment has been minimized.
(Procedurally, it'd be nice to reference the tracking issue in every PR related to that feature. Makes it much easier to navigate around for someone who's not following every single PR in this project.) |
f6c3c4b
to
082b0fa
Compare
This comment has been minimized.
This comment has been minimized.
082b0fa
to
3640f45
Compare
This comment has been minimized.
This comment has been minimized.
3640f45
to
3959c94
Compare
3959c94
to
da1df90
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
… r=<try> Ergonomic ref counting: optimize away clones when possible This PR build on top of rust-lang#134797. It optimizes codegen of ergonomic ref-counting when the type being `use`d is only known to be copy after monomorphization. We avoid codening a clone and generate bitwise copy instead. RFC: rust-lang/rfcs#3680 Tracking issue: rust-lang#132290 Project goal: rust-lang/rust-project-goals#107 r? `@nikomatsakis` This PR could better sit on top of rust-lang#131650 but as it did not land yet I've decided to just do minimal changes. It may be the case that doing what I'm doing regress the performance and we may need to go the full route of rust-lang#131650. cc `@saethlin` in this regard.
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (c5e50d7): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 8.1%, secondary 1.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 1.5%, secondary 3.8%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 774.818s -> 781.377s (0.85%) |
This PR build on top of #134797. It optimizes codegen of ergonomic ref-counting when the type being
use
d is only known to be copy after monomorphization. We avoid codening a clone and generate bitwise copy instead.RFC: rust-lang/rfcs#3680
Tracking issue: #132290
Project goal: rust-lang/rust-project-goals#107
r? @nikomatsakis
This PR could better sit on top of #131650 but as it did not land yet I've decided to just do minimal changes. It may be the case that doing what I'm doing regress the performance and we may need to go the full route of #131650.
cc @saethlin in this regard.