-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Specialize derive(Clone)
to use Copy
when applicable
#95668
Conversation
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
679a9d4
to
e90c1f6
Compare
Could be perf sensitive, so: @bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 95183ec with merge 074fe5f224a2ee59233947b06ddfd86252e1086c... |
☀️ Try build successful - checks-actions |
Queued 074fe5f224a2ee59233947b06ddfd86252e1086c with parent 306ba83, future comparison URL. |
Finished benchmarking commit (074fe5f224a2ee59233947b06ddfd86252e1086c): comparison url. Summary:
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking 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 led 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 Footnotes |
Do you need me to review this prior to fixing perf? |
Yes, I think perf might be unavoidable. |
What's the point of this, if not perf? |
compiler perf will definitely be negatively affected since we now emit two items for the derive. The question would be, "are there people who implement If there is a significant amount of them, then I think this would definitely be beneficial. But now it doesn't seem to have much of a benefit. Maybe we could discuss the possibility of doing something like this with perfect derives. |
Note that in the case of something like #[derive(Clone)]
pub struct Foo(usize, HashSet<String>, usize); The mir-opt in #94276 will replace the So that along with LLVM being pretty good at inlining the clones enough to notice they're actually copies (even the MIR inliner can often do this, though it's not on yet), it's possible that most of the low-hanging fruit here has already been picked. |
In this PR I added two internal items in
core::clone
:try_copy
andDerivedClone
.try_copy
will copy from&T
toT
if it can, and call a function when it cannot.The derive impl for
Clone
is changed such that it generates a call totry_copy(self, DerivedClone::clone)
, and the impl forDerivedClone
contains the derive impl before this PR.Internally:
I added a field to
TraitDef
namedbound_current_trait
that disables addition of generic bounds of the trait that is being implemented. This prevents bounding onDerivedClone
when we are generating that impl.