Repository navigation
Remove the grow path from the array merge loops - #385
Open
emschwartz wants to merge 2 commits into
Open
emschwartz wants to merge 2 commits into
emschwartz wants to merge 2 commits into
Conversation
With the `simd` feature, `ArrayStore -= &ArrayStore` reserved `min(lhs.len(), rhs.len())` for its output, but a difference can hold up to `lhs.len()` values. The Vec grew to cover it, so results were correct. Reserve `lhs.len()`, as `Sub` already does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every caller reserves an upper bound for its output, so the `push` in `visit_scalar` never reallocates, but the compiler cannot see that. The scalar merge loops for `&`, `|`, `^` and `-` therefore carry a call to `grow_one` and reload the Vec's fields from the stack on each write. Asserting `len < capacity` before the push removes the call and lets the loops keep their state in registers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 11, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
VecWriter::visit_scalarcallsVec::push, so the scalar merge loops for&,|,^and-on array containers each carry a call toRawVec::grow_one, and on x86-64 they keep the Vec's fields and both loop bounds on the stack. Every caller already reserves room for the whole output, so assertinglen < capacitybefore the push lets the compiler drop the call and keep the loop in registers.Two commits:
simd,ArrayStore -= &ArrayStorereservedmin(lhs.len(), rhs.len()), but a difference can holdlhs.len()values. The Vec grew to cover it, so results were right, but it would trip the assert. The added test panics on the second commit without this fix.VecWriterfrom a silent reallocation into a panic, which is now stated onVecWriter::new. The other five call sites reserve a true upper bound (l + rfor|and^,minfor&,lfor-).Existing
pairwise_*/ref_refbenches, change in time with a 95% interval. Each cell pools the 8 datasets over 10 interleaved runs of each binary. The last row is the unpatched binary against a copy of itself.&|^-So this is an x86-64 change. On the M2 it is close to neutral: single datasets range from -4.6% to +1.8% (
pairwise_subon census1881 is the slower one), and the aarch64 loop only loses 3 to 4 instructions per element written. The worst case found there is a synthetic union of 10 values with 4000, not part of these benches, at 7% slower (58% faster on the EPYCs). On the EPYCs every dataset is faster, census1881_srt by the least at 0 to 5%.Stable builds, default flags, rustc 1.99.0 on the EPYCs (dedicated Linode vCPUs, pinned to one) and 1.98.1 on the M2. With
--features simdon nightly 1.101.0 (3 runs),|and^are 7 to 10% faster and-4% faster on the EPYCs; no change was detected for&or on the M2, to within about 5%.Alternatives considered:
unsafeconstructor. Over 3 runs it showed no gain over the assert on any of the three machines, and a wrong bound would then corrupt memory instead of panicking.VecWriter::for_and(lhs, rhs)and so on) so that callers never pass a number. This would sit on top of the assert and is left out to keep the diff small; it can follow if wanted.Authored with Claude (claude-opus-5-5).