Skip to content

shrink_to_fit shrinks in place #50742

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

Closed
wants to merge 1 commit into from
Closed

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented May 14, 2018

This PR updates the documentation of Vec::shrink_to_fit to indicate that the request to shrink the vector's capacity is non-binding.

It used to say that "it shrinks the capacity as much as possible" but that was a lie since it also said "the allocator might inform the vector that there is space for a few more elements". Implementation-wise it called realloc which does not shrink the capacity as much as possible.

The only way to really shrink the capacity "as much as possible", is to perform a new allocation of the desired capacity, and move the elements over to it. This is not what was intended by the previous documentation.

The new docs more clearly state the "spirit" of shrink_to_fit, which is to reduce the unused capacity of the vector in whichever way it makes sense. This will depend, among other things, on the vector's allocator

The way this is now implemented in RawVec is to use shrink_in_place. Once PR #50738 is merged this can be updated to use shrink_in_place_excess.

@rust-highfive
Copy link
Contributor

r? @alexcrichton

(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 May 14, 2018
@gnzlbg
Copy link
Contributor Author

gnzlbg commented May 14, 2018

r? @sfackler


cc: @glandium @SimonSapin

The current implementation falls back to realloc if shrink_in_place fails. I am unsure whether this is even necessary.

I find it very weird that we have both shrink_in_place and grow_in_place methods instead of just realloc_in_place. The shrink_in_place method can return a CannotReallocInPlace error, which I find weird too. Is it a possible conforming implementation of shrink_in_place to always return CannotReallocInPlace ? If so, could shrink_to_fit just oom!() in that case ? Also, why would an allocator return CannotReallocInPlace instead of just not shrinking the allocation and returning success? There is a lot of documentation for these functions, but these issues aren't really covered.

Then, even when shrink_in_place fails, realloc could also fail returning an AllocError. How could that even happen when we are trying to shrink the capacity, and why does it currently produce an oom! error? We are after all trying to free some memory here.

@sfackler
Copy link
Member

Why doesn't realloc shrink the allocation as much as possible?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented May 14, 2018

Why doesn't realloc shrink the allocation as much as possible?

realloc returns an allocation that has a size that's at least as large as the one required. Therefore, a valid (and also the fastest) implementation of realloc if the size is smaller than the size of the current allocation is to just do nothing.

@sfackler
Copy link
Member

Do those implementations exist?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented May 14, 2018

Do those implementations exist?

EDIT: ~~~Of course, that's what for example jemalloc does. ~~! Wait, no, it only does that if the sizes are similar enough: https://github.com/jemalloc/jemalloc/blob/0fadf4a2e3e629b9fa43888f9754aea5327d038f/src/arena.c#L1652

Also, glibc does a similar thing, avoiding the reallocation only if the difference between the sizes isn't large enough: https://github.com/bminor/glibc/blob/master/malloc/malloc.c#L3206


We could do some sort of small size different optimization in the default implementation of the Alloc::realloc as well.

@gnzlbg gnzlbg closed this May 14, 2018
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.

4 participants