Skip to content
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

Restore original implementation of Vec::retain #67300

Merged
merged 1 commit into from
Dec 15, 2019

Conversation

aloucks
Copy link
Contributor

@aloucks aloucks commented Dec 14, 2019

This PR reverts #48065, which aimed to optimize Vec::retain by making use of Vec::drain_filter. Unfortunately at that time, drain_filter was unsound.

The soundness hole in Vec::drain_filter was fixed in #61224 by guaranteeing that cleanup logic runs via a nested Drop, even in the event of a panic. Implementing this nested drop affects codegen (apparently?) and results in slower code.

Fixes #65970

This PR reverts rust-lang#48065, which aimed to optimize `Vec::retain` by
making use of `Vec::drain_filter`. Unfortunately at that time,
`drain_filter` was unsound.

The soundness hole in `Vec::drain_filter` was fixed in rust-lang#61224 by
guaranteeing that cleanup logic runs via a nested `Drop`, even in
the event of a panic. Implementing this nested drop affects codegen
(apparently?) and results in slower code.

Fixes rust-lang#65970
@rust-highfive
Copy link
Collaborator

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 14, 2019
@Centril
Copy link
Contributor

Centril commented Dec 14, 2019

r? @Gankra cc @rkruppe @alexcrichton

@rust-highfive rust-highfive assigned Gankra and unassigned withoutboats Dec 14, 2019
@hanna-kruppe
Copy link
Contributor

r? @rkruppe

This is unfortunate, but it does seem to generate better code (I haven't measured performance but the assembly is a lot closer to what it was 1.38) and the correctness of the old implementation was never in question so I can rubber-stamp it.
It seems #48065 slightly changed behavior (relative order of predicate calls vs dropping elements) as a side effect. Naturally, that is also reverted by this PR. Given that this behavior change was discussed and considered OK back then, reverting it should be OK too.

@bors r+

@bors
Copy link
Contributor

bors commented Dec 14, 2019

📌 Commit 7ea6c46 has been approved by rkruppe

@rust-highfive rust-highfive assigned hanna-kruppe and unassigned Gankra Dec 14, 2019
@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 14, 2019
Centril added a commit to Centril/rust that referenced this pull request Dec 15, 2019
Restore original implementation of Vec::retain

This PR reverts rust-lang#48065, which aimed to optimize `Vec::retain` by making use of `Vec::drain_filter`. Unfortunately at that time, `drain_filter` was unsound.

The soundness hole in `Vec::drain_filter` was fixed in rust-lang#61224 by guaranteeing that cleanup logic runs via a nested `Drop`, even in the event of a panic. Implementing this nested drop affects codegen (apparently?) and results in slower code.

Fixes rust-lang#65970
bors added a commit that referenced this pull request Dec 15, 2019
Rollup of 6 pull requests

Successful merges:

 - #67255 (Remove i686-unknown-dragonfly target)
 - #67267 (Fix signature of `__wasilibc_find_relpath`)
 - #67282 (Fix example code of OpenOptions::open)
 - #67289 (Do not ICE on unnamed future)
 - #67300 (Restore original implementation of Vec::retain)
 - #67305 (Doc typo)

Failed merges:

r? @ghost
@bors bors merged commit 7ea6c46 into rust-lang:master Dec 15, 2019
@oxalica oxalica mentioned this pull request Jan 17, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2021
Optimize Vec::retain

Use `copy_non_overlapping` instead of `swap` to reduce memory writes, like what we've done in rust-lang#44355 and `String::retain`.
rust-lang#48065 already tried to do this optimization but it is reverted in rust-lang#67300 due to bad codegen of `DrainFilter::drop`.

This PR re-implement the drop-then-move approach. I did a [benchmark](https://gist.github.com/oxalica/3360eec9376f22533fcecff02798b698) 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 (rust-lang#52267) complains about it before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using retain() on a vector a lot slower in version 1.38.0 than 1.36.0
7 participants