-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Optimize Vec::retain #81126
Optimize Vec::retain #81126
Conversation
r? @kennytm (rust-highfive has picked a reviewer for you, use r? to override) |
The job Click to see the possible cause of the failure (guessed by this bot)
|
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.
Thanks for working on this!
I have a few comments:
@m-ou-se Fixed. Also updated the benchmark result. |
@bors r+ |
📌 Commit 2a11c57 has been approved by |
Thanks for working on this! |
☀️ Test successful - checks-actions |
Mirror optimization of std::Vec::retain (rust-lang/rust#81126)
Use
copy_non_overlapping
instead ofswap
to reduce memory writes, like what we've done in #44355 andString::retain
.#48065 already tried to do this optimization but it is reverted in #67300 due to bad codegen of
DrainFilter::drop
.This PR re-implement the drop-then-move approach. I did a benchmark on small-no-drop, small-need-drop, large-no-drop elements with different predicate functions. It turns out that the new implementation is >20% faster in average for almost all cases. Only 2/24 cases are slower by 3% and 5%. See the link above for more detail.
I think regression in may-panic cases is due to drop-guard preventing some optimization. If it's permitted to leak elements when predicate function of element's
drop
panic, the new implementation should be almost always faster than current one.I'm not sure if we should leak on panic, since there is indeed an issue (#52267) complains about it before.