Skip to content

Add slow vector initializations lint #3365

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

Merged
merged 7 commits into from
Nov 26, 2018

Conversation

gnieto
Copy link
Contributor

@gnieto gnieto commented Oct 26, 2018

Closes #3237

Add lint to detect slow zero-filled vector initialization. It detects
when a vector is zero-filled with extended with repeat(0).take(len)
or resize(len, 0).
This zero-fillings are usually slower than simply using vec![0; len]

@gnieto gnieto force-pushed the lint/slow-initialization branch from 63b5bab to b38ed6b Compare October 26, 2018 22:48
@phansch phansch added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 27, 2018
error: detected slow zero-filling initialization
--> $DIR/slow_vector_initialization.rs:67:5
|
67 | let mut unsafe_vec: Vec<u8> = Vec::with_capacity(200);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling this code "zero-filling initialization" may be misleading. with_capacity ends up in Alloc::alloc which does not ensure that the allocated memory is zero filled (unlike alloc_zeroed), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right, is misleading. I'm working on splitting the current lint into two: One for slow initialization and another for the unsafe.
I'll push the change tomorrow.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a change which splits the previous lint into two: One for slow and one for slow initializations.

@gnieto gnieto force-pushed the lint/slow-initialization branch from 7cd3f4d to 7c79a8a Compare October 30, 2018 21:34
@phansch
Copy link
Member

phansch commented Oct 31, 2018

retriggering CI

@phansch phansch closed this Oct 31, 2018
@phansch phansch reopened this Oct 31, 2018
@gnieto gnieto force-pushed the lint/slow-initialization branch 3 times, most recently from f5fac7c to acc23ac Compare November 5, 2018 20:56
@ghost
Copy link

ghost commented Nov 18, 2018

This looks good but seems like the it doesn't account for statements between initialization and extension.

For example,

fn do_stuff(vec: &mut Vec<u8>) {
    // ...
}

fn extend_vector_with_manipulations_between() {
    let len = 300;
    let mut vec1:Vec<u8> = Vec::with_capacity(len);
    do_stuff(&mut vec1);
    vec1.extend(repeat(0).take(len));
}

I'm pretty sure that will lead to false positives.
I think it might be best to use the approach used in the almost_swapped lint and just check two consecutive statements.

@gnieto
Copy link
Contributor Author

gnieto commented Nov 18, 2018

Thanks @mikerite for the review.

I'll change the behavior and only check consecutive statements.

@gnieto gnieto force-pushed the lint/slow-initialization branch from 8527beb to 692078b Compare November 18, 2018 22:56
@gnieto
Copy link
Contributor Author

gnieto commented Nov 18, 2018

@mikerite I've added a new commit which only checks the following (relevant) expression. I've tried to mimic the style on almost_swapped lint, but there were some edge cases that didn't work.

@gnieto gnieto force-pushed the lint/slow-initialization branch 2 times, most recently from 5e82e2d to 946dbb6 Compare November 20, 2018 22:00
@ghost
Copy link

ghost commented Nov 25, 2018

I wanted to merge this now but I found that don't have write access any more. Hopefully, someone with access will pick this up.

@flip1995
Copy link
Member

This needs a rebase and maybe another run of util/dev update_lints.

gnieto and others added 7 commits November 25, 2018 14:34
Add lint to detect slow zero-filled vector initialization. It detects
when a vector is zero-filled with extended with `repeat(0).take(len)`
or `resize(len, 0)`.
This zero-fillings are usually slower than simply using `vec![0; len]`.
Renamed some symbols in order to make them a little bit more accurate.
Instead of searching for all the successive expressions after a vector
allocation, check only the first expression.

This is done to minimize the amount of false positives of the lint.
@gnieto gnieto force-pushed the lint/slow-initialization branch from 946dbb6 to dc35841 Compare November 25, 2018 22:36
@gnieto
Copy link
Contributor Author

gnieto commented Nov 25, 2018

Rebased and ran again util/dev update_lints

@oli-obk oli-obk merged commit 1e2291c into rust-lang:master Nov 26, 2018
@gnieto gnieto deleted the lint/slow-initialization branch November 26, 2018 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants