Optimize Vec push by preventing address escapes

[gerben-stavenga]

View all comments

This change makes RawVecInner non-generic over the allocator, allowing it to be Copy. The allocator is moved to RawVec itself. Key optimizations:

• RawVecInner is now Copy (no allocator field)
• grow_one uses ptr::read/ptr::write to copy allocator to a temporary, preventing &self from escaping through &dyn Allocator parameter
• Drop::drop similarly copies to temporaries before deallocating
• deallocate takes self by value instead of &mut self
• All these functions are #[inline(always)]

This allows LLVM to keep Vec fields (cap, ptr, len) in registers during push loops instead of storing/loading from memory every iteration.

Benchmark results (push with pre-allocated capacity):

• 100 elements: 1.74x faster
• 1000 elements: 1.87x faster
• 10000 elements: 2.41x faster

Secondary benefit: grow_one_impl and other growth functions use &dyn Allocator, so they are compiled once in libstd rather than monomorphized per allocator type.

Preserves const compatibility with the const_heap feature by using generics for the const allocation path while using &dyn Allocator for runtime paths.

[saethlin]

• All these functions are #[inline(always)]

Please try to only use that attribute where it is demonstrated to be better than #[inline].

Secondary benefit: grow_one_impl and other growth functions use &dyn Allocator, so they are compiled once in libstd rather than monomorphized per allocator type.

Isn't this a penalty for small custom allocators that can be inlined?

[gerben-stavenga]

• All these functions are #[inline(always)]

Please try to only use that attribute where it is demonstrated to be better than #[inline].

These are on xxx(&mut self) functions that forward to functions that take by self and return self. If not always inlined the &mut self escapes a reference, producing code that is drastically worse. The inner loop in the benchmarks with this PR is

bf330: mov %r13,(%rdx,%r13,8) ; vec[len] = len
bf334: inc %r13 ; len++
bf337: cmp %r13,%r15 ; compare with target
bf33a: je bf370 ; done if equal
bf33c: cmp %rax,%r13 ; compare with capacity
bf33f: jne bf330 ; loop back

before:

bf600: mov -0x38(%rbp),%rax ; LOAD ptr from stack
bf604: mov %r15,(%rax,%r15,8) ; vec[len] = len
bf608: inc %r15 ; len++
bf60b: mov %r15,-0x30(%rbp) ; STORE len to stack
bf60f: cmp %r15,%r14 ; compare with target
bf612: je bf630 ; done if equal
bf614: cmp -0x40(%rbp),%r15 ; LOAD capacity from stack
bf618: jne bf600 ; loop back

Secondary benefit: grow_one_impl and other growth functions use &dyn Allocator, so they are compiled once in libstd rather than monomorphized per allocator type.

Isn't this a penalty for small custom allocators that can be inlined?

I suspect there is a small penalty due to indirection (although the compiler seem to generate call reg in the direct case too). But there are also positive side effects due to code dedup. These are fallback paths so from that perspective a tiny regression isn't the worst. The point of this PR is that the existence of fallback path should not influence the compilers ability to optimize the fast path and keep that clean and tight.

The &dyn Allocator change can be changed to &Allocator at the cost of monomorphizing grow function.

[rustbot]

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[tgross35]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[rust-bors]

⌛ Trying commit ac22726 with merge b00b54d…

To cancel the try build, run the command @bors try cancel.

Workflow: https://github.com/rust-lang/rust/actions/runs/20890110771

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[Noratrieb]

FWIW the compile time benchmarks will not say anything about the runtime perf of this change, since such large refactorings of Vec are pretty much guaranteed to have some compile time impact on crates that use vec (so every crate).
So the compile time impact of this change and the runtime impact on the vecs in the compiler will be hard to untangle.

[hkBst]

I don't see any changes that could warrant this function changing to no longer being unsafe, it even still has the same safety comments...

[hkBst]

Please remove these AI comments.

[hkBst]

same here

[saethlin]

If not always inlined the &mut self escapes a reference, producing code that is drastically worse.

When you say "always inlined" are you referring to the attribute or the optimization?

[tgross35]

Could you define the “escaping” term that you keep using? In Rust I’m only aware of that referring to lifetimes, which isn’t relevant for codegen.

[gerben-stavenga]

Could you define the “escaping” term that you keep using? In Rust I’m only aware of that referring to lifetimes, which isn’t relevant for codegen.

In compiler analysis the most important step is mem2reg, ie. lower stack variables to register. The crucial part of this step is that there is no reference to the stack variable. So local function variables whose stack address does not escape the compiler analysis (for example by passing it to some other function) can easily be moved to SSA variables. This allows subsequent codegen to keep those variables in register (because there is no need to sync the register to stack). So currently the reference to self (len, cap, ptr) is passed to grow and thus all subsequent codegen is poluted by unnecessary register <-> stack syncing (see the asm i posted). This PR removes the reference to stack variables from the outline function call. in lieu of passing and returning cap, ptr by value (ie. register)

[gerben-stavenga]

If not always inlined the &mut self escapes a reference, producing code that is drastically worse.

When you say "always inlined" are you referring to the attribute or the optimization?

I refer to the optimization, it's crucial that functions taking &mut self, are always inlined because then compiler optimization will see that the reference can eliminated.

[gerben-stavenga]

FWIW the compile time benchmarks will not say anything about the runtime perf of this change, since such large refactorings of Vec are pretty much guaranteed to have some compile time impact on crates that use vec (so every crate). So the compile time impact of this change and the runtime impact on the vecs in the compiler will be hard to untangle.

I'm not sure if I understand you. The main point of this PR is runtime performance of code. I hope the benchmarks I'm running are the benchmarks for measuring the runtime perf of the Vec implementation.

There might be a compile time benefit. Because the fallback grow function is only compiled once as part of the standard lib and not, like the current state, in each crate that uses vec.

[saethlin]

I refer to the optimization, it's crucial that functions taking &mut self, are always inlined because then compiler optimization will see that the reference can eliminated.

I do not think this is sufficient justification for inline(always). We have so many functions which would cause similar or worse optimization degradation if they weren't inlined in optimized builds. This just isn't worth the cost of the degradation to debug build times that is caused by inline(always). If #[inline] suffices, always is all cost no benefit.

[tgross35]

^ to reiterate that, the rule of thumb now is that any use of #[inline(always)] needs to be backed up by benchmarks and codegen showing it makes a meaningful difference over ‘#[inline]. ‘#[inline(always)]` hurts unoptimized builds and size-optimized binaries so we need to be very cautious with its use.

In general here, it would be helpful if you could put a mini version of the before and after code on godbolt so we can get the bigger picture of what’s actually happening at the different levels.

[gerben-stavenga]

^ to reiterate that, the rule of thumb now is that any use of #[inline(always)] needs to be backed up by benchmarks and codegen showing it makes a meaningful difference over ‘#[inline]. ‘#[inline(always)]` hurts unoptimized builds and size-optimized binaries so we need to be very cautious with its use.

In general here, it would be helpful if you could put a mini version of the before and after code on godbolt so we can get the bigger picture of what’s actually happening at the different levels.

https://godbolt.org/z/nrnP4T83e

shows a rather minimal version, you can see the difference in codegen the test functions

vec_push vs rf_push

[JohnCSimon]

Ping from triage: @gerben-stavenga - can you post your status on this PR? I'll be closing because of inactivity otherwise. Thanks

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[gerben-stavenga]

Ping from triage: @gerben-stavenga - can you post your status on this PR? I'll be closing because of inactivity otherwise. Thanks

Hi,

I re-synced the pr and fixed some const-eval issues.

The basic optimization is I think good. Ensuring that all relevant state is passed-in and returned by value is good and can drastically improve codegen as shown by the benchmarks.

The type-erasure is something I like. De-duplicating the same outline coldish resizing code is something I like, but willing to remove. It's all up to the maintainers. If you decide its worthwhile to pursue I'm willing to update the PR as you see fit. If not close it out.

[rust-bors]

☔ The latest upstream changes (presumably #161990) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.