Fix excessive memory allocation in RawVec::reserve #29454
Conversation
|
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
|
☔ The latest upstream changes (presumably #29452) made this pull request unmergeable. Please resolve the merge conflicts. |
stepancheg
force-pushed
the
vec-reserve
branch
from
4e03a2b
to
09413c3
Compare
|
Rebased. |
|
I like the new logic and can't see any obvious problems with it. |
|
If we're making changes here I think a growth factor of 1.5 would be better than 2 although I'm not sure if using jemalloc changes things here. This blog post explains the reasoning but basically with 2 you are never able to reuse an old allocation. |
|
@amaranth grow factor is a separate, completely independent issue (I also think 1.5 would be better).
Only if your application has only one |
|
Should we run some benches to make sure there aren't any issues with this? This is a no-inline cold path, so I don't think it's really necessary, but I'm not sure. |
|
@gankro I don't think bench is necessary, because code is straightforward, and it's similar to standard
Actually, it is not cold path, for example, when you have thousands of vectors of 2..5 elements. However, I think time spent in malloc it much more than a half of time spent in |
|
Using |
|
r? @bluss You seem to have a good handle on the issues at play here. I skimmed the code but am a bit fuzzy on if this will have the same crash behaviour when inserting into ZST collections. In particular, they rely on RawVec to crash to elide their own overflow checks for |
|
@bluss |
|
@gankro Ok, way to pass a hot potato! 😄 From my reading, the behavior for ZST is exactly the same. Either the capacity fits and it returns, or it panics for capacity overflow. I'll leave comments inline. |
| // If we make it past the first branch then we are guaranteed to | ||
| // panic. | ||
| let required_cap = used_cap.checked_add(needed_extra_cap) | ||
| .expect("capacity overflow"); |
There was a problem hiding this comment.
We must move this overflow check and panic case to below the required_cap <= self.cap check. While preserving this panic if it's a ZST element type.
The reason is that the fast path is the section of code before we decide if we need to reallocate or not, and it should be free from a branch to panic.
There was a problem hiding this comment.
The first if in the original code is fine to use.
There was a problem hiding this comment.
That's a good point. I overlooked it.
Before this patch `reserve` function allocated twice as requested
amount elements (not twice as capacity). It leaded to unnecessary
excessive memory usage in scenarios like this:
```
let mut v = Vec::new();
v.push(17);
v.extend(0..10);
println!("{}", v.capacity());
```
`Vec` allocated 22 elements, while it could allocate just 11.
`reserve` function must have a property of keeping `push` operation
cost (which calls `reserve`) `O(1)`. To achieve this `reserve` must
exponentialy grow its capacity when it does reallocation.
There's better strategy to implement `reserve`:
```
let new_capacity = max(current_capacity * 2, requested_capacity);
```
This strategy still guarantees that capacity grows at `O(1)` with
`reserve`, and fixes the issue with `extend`.
Patch imlpements this strategy.
stepancheg
force-pushed
the
vec-reserve
branch
from
09413c3
to
612d405
Compare
|
Updated patch with fast-path branch made first as before. |
stepancheg
force-pushed
the
vec-reserve
branch
2 times, most recently
from
474c20c
to
46068c9
Compare
|
Thank you! This is beautiful. @bors r+ |
|
📌 Commit 46068c9 has been approved by |
Before this patch `reserve` function allocated twice as requested
amount elements (not twice as capacity). It leaded to unnecessary
excessive memory usage in scenarios like this:
```
let mut v = Vec::new();
v.push(17);
v.extend(0..10);
println!("{}", v.capacity());
```
`Vec` allocated 22 elements, while it could allocate just 11.
`reserve` function must have a property of keeping `push` operation
cost (which calls `reserve`) `O(1)`. To achieve this `reserve` must
exponentialy grow its capacity when it does reallocation.
There's better strategy to implement `reserve`:
```
let new_capacity = max(current_capacity * 2, requested_capacity);
```
This strategy still guarantees that capacity grows at `O(1)` with
`reserve`, and fixes the issue with `extend`.
Patch imlpements this strategy.
bluss
added
the
relnotes
Before this patch
reservefunction allocated twice as requestedamount elements (not twice as capacity). It leaded to unnecessary
excessive memory usage in scenarios like this:
Vecallocated 22 elements, while it could allocate just 11.reservefunction must have a property of keepingpushoperationcost (which calls
reserve)O(1). To achieve thisreservemustexponentialy grow its capacity when it does reallocation.
There's better strategy to implement
reserve:This strategy still guarantees that capacity grows at
O(1)withreserve, and fixes the issue withextend.Patch imlpements this strategy.