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

slow-vector-initialization should catch Vec::new followed by Vec::resize(len, 0). #10938

Closed
JarredAllen opened this issue Jun 12, 2023 · 4 comments · Fixed by #11198
Closed
Labels
C-bug Category: Clippy is not doing the correct thing I-false-negative Issue: The lint should have been triggered on code, but wasn't

Comments

@JarredAllen
Copy link
Contributor

JarredAllen commented Jun 12, 2023

Summary

There's a lint, slow-vector-initialization, which catches if you try to do something like this:

let mut foo = Vec::with_capacity(1024);
foo.resize(1024, 0);

It suggests you use the vec! macro instead, which is faster. However, if you replace the call to Vec::with_capacity with Vec::new, you get an even worse-performing version of the same code, and clippy is no longer able to suggest the better option.

Lint Name

slow-vector-initialization

Reproducer

I tried this code:

pub fn main() {
    let mut bar = Vec::new();
    bar.resize(1024, 0);
}

I expected to see this happen:

Raise an instance of slow-vector-initialization, something similar to this:

warning: slow zero-filling initialization
 --> src/main.rs:3:5
  |
2 |     let mut bar = Vec::new();
  |                   ------------------ help: consider replace allocation with: `vec![0; 1024]`
3 |     bar.resize(1024, 0);
  |     ^^^^^^^^^^^^^^^^^^^
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#slow_vector_initialization
  = note: `#[warn(clippy::slow_vector_initialization)]` on by default

Instead, this happened:

No lint raised

Version

rustc 1.70.0 (90c541806 2023-05-31)
binary: rustc
commit-hash: 90c541806f23a127002de5b4038be731ba1458ca
commit-date: 2023-05-31
host: aarch64-apple-darwin
release: 1.70.0
LLVM version: 16.0.2

(this also happens on the version running on playground right now)
@JarredAllen JarredAllen added C-bug Category: Clippy is not doing the correct thing I-false-negative Issue: The lint should have been triggered on code, but wasn't labels Jun 12, 2023
@bors bors closed this as completed in 70c5798 Jul 25, 2023
@vi
Copy link
Contributor

vi commented Aug 3, 2023

Why aren't vec![0; 1024], Vec::new() + resize(0, 1024) and Vec::with_capacity(1024) + resize(0, 1024) all the same?
Isn't number of memory allocations and number of zero filling sweeps the same in all versions?

Or is it some special properly of 0 and it would indeed be all the same with 1 as the value?

@y21
Copy link
Member

y21 commented Aug 3, 2023

Isn't number of memory allocations and number of zero filling sweeps the same in all versions?

I don't think so. Last I checked, vec![0; x] uses alloc_zeroed, while .resize(x, 0) compiles to a (re)alloc + memset.
But yes, it would be the same for any other value than 0.

@djc
Copy link
Contributor

djc commented Aug 9, 2023

It would be nice to get this explained more in https://rust-lang.github.io/rust-clippy/master/index.html#/slow_vect. Right now the only examples it gives are about initializations with with_capacity() (as opposed to new()), and I definitely agree with @vi that my mental model is that it's the number of allocations that counts here, so it would be good to explain the performance impact a bit more for this case.

@y21
Copy link
Member

y21 commented Aug 9, 2023

Good point, this should be documented. I'll put up a PR for this in a bit

bors added a commit that referenced this issue Aug 9, 2023
…,djc

[`slow_vector_initialization`]: clarify why `Vec::new()` + resize is worse

#11198 extended this lint to also warn on `Vec::new()` + `resize(0, len)`, but did not update the lint documentation, so it left some confused (#10938 (comment)).
This PR should make it a bit more clear. (cc `@djc` `@vi` what do you think about this?)

<details>
<summary>More details</summary>

Godbolt for `Vec::new()` + `.resize(x, 0)`: https://godbolt.org/z/e7q9xc9rG

The resize call first does a normal allocation (`__rust_alloc`):
```asm
alloc::raw_vec::finish_grow:
  ...
  cmp     qword ptr [rcx + 8], 0
  je      .LBB1_7  ; if capacity == 0 -> LBB1_7

.LBB1_7:
  ...
  call    qword ptr [rip + __rust_alloc@GOTPCREL]
```

*Then* a memset for zero initialization:
```asm
example::f:
  ...
  xor     esi, esi  ; 0
  call    qword ptr [rip + memset@GOTPCREL]
```
------------

Godbolt for `vec![0; len]`: https://godbolt.org/z/M3vr53vWY

Important bit:
```asm
example::f:
  ...
  call    qword ptr [rip + __rust_alloc_zeroed@GOTPCREL]
```

</details>

changelog: [`slow_vector_initialization`]: clarify why `Vec::new()` + resize is worse than `vec![0; len]`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-negative Issue: The lint should have been triggered on code, but wasn't
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants