-
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
[Experiment] [WIP] Add clone_from to clone derive #98445
[Experiment] [WIP] Add clone_from to clone derive #98445
Conversation
r? @davidtwco (rust-highfive has picked a reviewer for you, use r? to override) |
During last attemp ( #27939 ), there wasn't perf run so we can make more well-founded decision about implementing |
The job Click to see the possible cause of the failure (guessed by this bot)
|
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 861872e with merge f39c353892b43777fbb6a39fd8a2004d9c6b1499... |
FYI: I'm also in the middle of fiddling with derive code: #98190, #98376, #98446. The first two are r+ and in the queue to merge. It's possible there will be conflicts, hopefully nothing too bad. #98376 removed the generated |
@nnethercote I think, Can you explain how you measured compile time wins? I would try to do that too. |
It's already happening :) This PR is currently in the perf. bot queue, once it's done, we will get a comment here with the results of compile time benchmarks. |
I'll note that this would change the panic safety of it -- the So I'll toss this on the @rust-lang/libs-api radar in case they have thoughts about whether it should happen from a semantic perspective (separate from implementation/perf questions). |
We already don't exception safe in some important So, probably, we should just document in trait definition that |
☀️ Try build successful - checks-actions |
Queued f39c353892b43777fbb6a39fd8a2004d9c6b1499 with parent fc96600, future comparison URL. |
Finished benchmarking commit (f39c353892b43777fbb6a39fd8a2004d9c6b1499): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
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 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 Footnotes |
Worst case regression (264.5% on issue-58319) is looks like made specifically against |
I think, benchmarking of the compiler doesn't so relevant to average rust program in that case because compiler source already has manually optimized by adding
IMHO, that would describe expected changes more accurately for average program. |
I'm not really sure what do you mean by that. The perf. suite that was executed checks how long does it take to compile various crates (both artificial stress tests and real-world crates like So, for example, if this PR was merged, it would take ~1-5% longer to compile crates like Maybe we can do something to optimize the implementation here, but it will probably be quite difficult. We also currently don't have infrastructure for benchmarking compiled programs, to see if the |
So... this is not about |
You mean in the call-site?
I mean, we would get regression from Maybe it is better just to run some real-world code. |
FWIW: a 100% result is a 100% increase in compile-time, i.e. a doubling in compilation time. A 264.5% regression is a 3.645x slowdown. It's a weirdly large slowdown. |
No, I mean the LLVM IR we generate for |
@nnethercote Thanks for your remark! I have just run profiling using It seems that borrow checker and typechecker really don't like Diff with base and my changes:
This values became much better when I manually rewrote Clone impl like #[inline]
fn clone(&self)->Self{
Self(::core::clone::Clone::clone(&self.0), ..., ::core::clone::Clone::clone(&self.613),)
}
#[inline]
fn clone(&self, other: &Self){
::core::clone::Clone::clone_from(&mut self.0, &other.0);
...
::core::clone::Clone::clone_from(&mut self.613, &other.613);
} Results
As you see, difference is nearly 10x on borrow check. You are removing match statement in #98446 so maybe it is good idea retry perf after you merge that? |
I already started an impl on this 👀 #97528 |
Regardless of performancec impact, I'm not sure we can do this, since this changes the clone_from behaviour in a potentially breaking way in the presence of panics, as @scottmcm already mentioned. |
Since we already have a Vec::clone_from that behaves differently than Vec::clone when stuff panics, it's indeed probably good to update that documentation. But I don't think that means it's fine to silently change the behavior of derived |
We discussed this in today's @rust-lang/libs-api meeting, and we decided not to make this change. People can hand-implement |
It isn't panic safe de-facto (e.g. for `Vec`) so I just document current behaviour. Panic safety was mentioned by @scottmcm when discussing [PR with deriving clone_from](rust-lang#98445 (comment)). Co-authored-by: Jane Losare-Lusby <jlusby42@gmail.com>
Fixes #98374
This is still just experiment to run perf, code needs some cleanup.