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

Using retain() on a vector a lot slower in version 1.38.0 than 1.36.0 #65970

Closed
Miguel-c-s opened this issue Oct 30, 2019 · 8 comments · Fixed by #67300
Closed

Using retain() on a vector a lot slower in version 1.38.0 than 1.36.0 #65970

Miguel-c-s opened this issue Oct 30, 2019 · 8 comments · Fixed by #67300
Labels
A-collections Area: `std::collection` A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-slow Issue: Problems and improvements with respect to performance of generated code. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Miguel-c-s
Copy link

Miguel-c-s commented Oct 30, 2019

I posted this on stackoverflow.

What happens is that when using retain over a vector, it seems to be a lot slower in version 1.38.0.
host is: x86_64-unknown-linux-gpu (according to rustc--version --verbose) and LLVM version is 8.0.

I'm using ubuntu 19.04.

As I wrote on SO, I tried:

use std::time::Instant;
fn main() {
    let start = Instant::now();
    let mut my_vec:Vec<u32>;
    for _ in 0..100_000{
        my_vec = (0..10_000).collect();
        my_vec.retain(|&x| x < 9000);
        my_vec.retain(|&x| x < 8000);
        my_vec.retain(|&x| x < 7000);
        my_vec.retain(|&x| x < 6000);
        my_vec.retain(|&x| x < 5000);
        my_vec.retain(|&x| (x < 5) & (x > 2));
    }
    let duration = start.elapsed();
    println!("Program took: {:?}", duration);
}

And changed between versions with rustup default 1.36.0 and rustup default stable, having updated rust before with rustup update.
p.s.: From my complete program I also noticed that version 1.38.0 was still a little bit slower without using retain(). But the difference was minimal (~5%) and hard to come up with another culprit.

@peterjoel
Copy link
Contributor

peterjoel commented Oct 30, 2019

Performance of this code in 1.37 is similar to that in 1.36, so this would appear to be a regression in 1.38.

  • Rust 1.37.0: 2.8 seconds
  • Rust 1.38.0: 5.7 seconds

@mati865
Copy link
Contributor

mati865 commented Oct 30, 2019

Minimal example: https://godbolt.org/z/f81qSU

@tesuji
Copy link
Contributor

tesuji commented Oct 30, 2019

Could you check against nightly compiler?

@Centril Centril added I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. labels Oct 30, 2019
@Miguel-c-s
Copy link
Author

In the nightly version it takes about the same time as in stable.

@tesuji
Copy link
Contributor

tesuji commented Oct 30, 2019

stable is Rust 1.38.0?

@Miguel-c-s
Copy link
Author

yes, stable is 1.38.0.

@Miguel-c-s
Copy link
Author

Miguel-c-s commented Dec 6, 2019

Will this ever be fixed or looked at? I understand that the devs may have too much work and too little help, I wish I had the knowledge to help, just sincerely asking.

@mati865
Copy link
Contributor

mati865 commented Dec 7, 2019

I bet it's fallout from upgrade to LLVM 9 but bisecting it would greatly help (either manually or with https://github.com/rust-lang/cargo-bisect-rustc/blob/master/TUTORIAL.md).

@jonas-schievink jonas-schievink added C-bug Category: This is a bug. A-collections Area: `std::collection` A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. labels Dec 12, 2019
Centril added a commit to Centril/rust that referenced this issue 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 bors closed this as completed in 7ea6c46 Dec 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Area: `std::collection` A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-slow Issue: Problems and improvements with respect to performance of generated code. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants