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

Possible Vec::truncate improvement #76089

Open
leonardo-m opened this issue Aug 29, 2020 · 5 comments
Open

Possible Vec::truncate improvement #76089

leonardo-m opened this issue Aug 29, 2020 · 5 comments
Labels
A-collections Area: `std::collection` E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@leonardo-m
Copy link

The docs of Vec::truncate say:

"If len is greater than the vector's current length, this has no effect."

And indeed if you look at the source code:
https://doc.rust-lang.org/nightly/src/alloc/vec.rs.html#740-742

There's:

if len > self.len {
    return;
}

But isn't it better to use len >= self.len instead?

@ecstatic-morse
Copy link
Contributor

See #74172.

@pickfire
Copy link
Contributor

This doesn't just improve truncate itself but also resize. Resize runs truncate even on the same size which runs the rest of the code.

The previous change results in an additional branch but we could still improve this if we could prevent that.

@camelid camelid added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 20, 2020
@workingjubilee workingjubilee added the E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. label Apr 24, 2021
@workingjubilee
Copy link
Member

workingjubilee commented Apr 24, 2021

#78884 and #74172 both attempted to fix this, but due to inlining rules it is more complex than it appears to be.

Quoting scottmcm:

For anyone else who arrives and looks at this: this can happen because the old code, after inlining, resulted in if 0 > some_unsigned_number, which is of course never possible and thus gets optimized away. Changing the code so that the check is >= makes it possible again, and thus it is no longer optimized out.

@workingjubilee workingjubilee added E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. and removed E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Apr 24, 2021
@the8472 the8472 added A-collections Area: `std::collection` and removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Feb 8, 2022
@ChrisDenton
Copy link
Member

#80857 added a comment to the code with an explanation of why we're doing this. Should this be closed now? For the reasons outlined above, changing this is not planned.

@toomuchsalt
Copy link

Shouldn't this be reviewed again, as clear doesn't call truncate after #96002 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Area: `std::collection` E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants