Swap self and other in vec::append when it avoids a reallocation #77538
Conversation
|
r? @kennytm (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
the8472
changed the title
kennytm
added
the
T-libs-api
|
r? @KodrAus |
| @@ -1257,6 +1257,14 @@ impl<T> Vec<T> { | |||
| #[inline] | |||
| #[stable(feature = "append", since = "1.4.0")] | |||
| pub fn append(&mut self, other: &mut Self) { | |||
There was a problem hiding this comment.
Or we could just put a big notice that possible allocation stealing in the docs of append to give users a note.
| /// `Vec` will not automatically shrink itself, even if completely empty, when doing so | ||
| /// would cause unnecessary allocations or deallocations to occur. Emptying a `Vec` |
There was a problem hiding this comment.
I believe playing with words would confuse most users, this does not feel much different to me. Maybe
| /// `Vec` will not automatically shrink itself, even if completely empty, when doing so | |
| /// would cause unnecessary allocations or deallocations to occur. Emptying a `Vec` | |
| /// `Vec` will not automatically shrink itself, even if completely empty. **But** it | |
| /// will try to reduce unnecessary allocations or deallocations by reusing existing allocation. Emptying a `Vec` |
Maybe a big but helps? Probably needs rewrapping.
| // The capacity limit ensures that we are not stealing a large preallocation from `other` | ||
| // that is not commensurate with the avoided reallocation in self. | ||
| if self.len == 0 && self.capacity() < other.len && other.capacity() / 2 <= other.len { | ||
| mem::swap(self, other); |
There was a problem hiding this comment.
Even though this helps the general case but it may regress on some cases where users want to reuse the capacity of the other Vec.
There was a problem hiding this comment.
without this change: it must allocate now to grow self
with this change: it may have to allocate later to grow other
|
A more conservative alternative would be to swap only if |
|
Thanks for putting so much effort into optimizing Did we have a motivating usecase for this? It would be good to understand some cases where we reduce allocations, and cases where we don't. |
|
And this could use a perf run to see if it even has any positive impact. |
|
I'm ok with that re-interpretation of no-shrinking to avoid re-allocating when it's not necessary. The reason I was interested in motivating cases is because it makes the performance characteristics a bit less local, so it's tricky to tell from just the input vectors whether it's an improvement or not. Let's do a timer run! @rust-timer queue |
|
Awaiting bors try build completion |
@bors try |
|
⌛ Trying commit 4cdbb8c with merge 43f40c41fe26c6cc769b75bf60734fb20e68a622... |
|
☀️ Try build successful - checks-actions |
|
Queued 43f40c41fe26c6cc769b75bf60734fb20e68a622 with parent 07a63e6, future comparison URL. |
|
Finished benchmarking try commit (43f40c41fe26c6cc769b75bf60734fb20e68a622): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
|
Meh, doesn't seem worthwhile. |
|
Thanks for investigating @the8472! |
Spun out from #77496
Implements #56763
This needs discussion since it slightly weakens the "no shrinking" guarantee, even though it keeps with the stated intent of the rule that