-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Performance improvement of Vec's swap_remove. #52166
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
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
src/liballoc/vec.rs
Outdated
// safe even when index == self.len() - 1, as pop() only uses | ||
// ptr::read and leaves the memory at self[index] untouched. | ||
let hole: *mut T = &mut self[index]; | ||
ptr::replace(hole, self.pop().unwrap()) |
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 can't help but feel that it would be clearer and require less explanation to first move the element with ptr::replace
, then decrease the size, rather than relying on the implementation of pop
.
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 agree - in fact I had that implementation at some point. However in the discussion of #52150 the emphasis was on less unsafe code, so I may have gone overboard a bit. I'll change it.
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'm less concerned about the quantity, and more concerned that the unsafe code we do have should be as straightforward and obvious as possible.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This seems much more straightforward to me. 👍 |
@bors r+ |
📌 Commit e529dfd has been approved by |
Performance improvement of Vec's swap_remove. The old implementation *literally* swapped and then removed, which resulted in unnecessary move instructions. The new implementation does use unsafe code, but is easy to see that it is correct. Fixes #52150.
☀️ Test successful - status-appveyor, status-travis |
Sorry, yeah, absolute number of unsafe widgets is clearly less important than the subtlety of their correctness (I mentioned that in my first comment in #52150 but was sloppy with it). However, I found the But speaking of unnecessary subtlety, the |
@rkruppe That's a good catch, and no less of a subtlety than the original formulation. I almost suggested a second simplification, which would have also avoided that, but I ended up dismissing the difference; clearly I shouldn't have. Rather than |
There is an explicit comment explaining that there must be a last element if the bounds check of
That subtlety is mentioned in the committed versions, and it's not just limited to
I considered it, but the assembly generated looked less favorable, and most importantly, I followed the implementation of |
Yes, but one still has to think that through (and verify that the code actually corresponds to what the comment describes), and it has no advantage either. Using an unchecked method where safe indexing would do needs to be motivated. (It also looks uglier, but that's only a very minor aspect.)
Good point about it applying whenever removing the last element, but my point is specifically about
I haven't looked at assembly myself, but this sounds plausible. Ideally we shouldn't need to worry about temporaries, but unfortunately our optimizations around temporaries and memcpys are still less than stellar. (In contrast, specializing |
let hole: *mut T = &mut self[index]; | ||
let last = ptr::read(self.get_unchecked(self.len - 1)); | ||
self.len -= 1; | ||
ptr::replace(hole, last) |
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 item on hole doesn't seem to be dropped with this change, does it?
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.
That item is returned.
The old implementation literally swapped and then removed, which resulted in unnecessary move instructions. The new implementation does use unsafe code, but is easy to see that it is correct.
Fixes #52150.