-
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
Replace Copy/Clone compiler magic on arrays with library impls #86041
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
The code building the clone shim for array can be removed now as well, right? |
601e975
to
4f2d9b4
Compare
I've addressed the feedback and this is ready for review. |
☔ The latest upstream changes (presumably #87781) made this pull request unmergeable. Please resolve the merge conflicts. |
Hi @bstrie! Would you mind rebasing this PR and resolving the merge conflict so we can do a perf run? Assuming that does not reveal any issues, I think we're about ready to merge this. |
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 LGTM to me on compiler and traits changes.
Ping from triage: |
This seems reasonable to me, once the merge conflicts are resolved and a perf run comes back without regressions. |
4f2d9b4
to
8a772b9
Compare
That looks better...mostly a bit more time spent in LLVM (not completely unexpected). Now this is even a slight improvement in a couple benchmarks. With the latest results, the balance has shifted away from an intrinsic or some other magic being a good idea, imo. |
I think performance results are definitely closer to being acceptable here, I'm OK with not bothering with trying out the shim approach or hand-coding it to generate better IR. |
Maybe we could write a simple specialized function for fn clone(&self) -> Self {
let mut array = MaybeUninit::uninit_array::<N>();
let mut guard = Guard { array_mut: &mut array, initialized: 0 };
for i in 0..N {
let item = src[i].clone();
guard.array_mut[i].write(item);
guard.initialized = i;
}
mem::forget(guard);
unsafe { MaybeUninit::array_assume_init(array) }
} |
Imo, i think this would better be left as followup. |
Okay, given previous review from @joshtriplett, previous discussion during a compiler meeting, and perf review by @Mark-Simulacrum, and my own review of compiler/traits changes, I'm going to go ahead and @bors r+ |
📌 Commit 61b1394 has been approved by |
⌛ Testing commit 61b1394 with merge fc59b7e59222dafbb1811b9c34659bf35deff255... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
@bors retry |
☀️ Test successful - checks-actions |
Finished benchmarking commit (d608229): comparison url. Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression |
With const generics the compiler no longer needs to fake these impls.