Skip to content

Remove the grow path from the array merge loops - #385

Open
emschwartz wants to merge 2 commits into
RoaringBitmap:mainfrom
emschwartz:vecwriter-no-grow
Open

emschwartz wants to merge 2 commits into
RoaringBitmap:mainfrom
emschwartz:vecwriter-no-grow

Conversation

@emschwartz

Copy link
Copy Markdown

VecWriter::visit_scalar calls Vec::push, so the scalar merge loops for &, |, ^ and - on array containers each carry a call to RawVec::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 asserting len < capacity before the push lets the compiler drop the call and keep the loop in registers.

Two commits:

  1. With simd, ArrayStore -= &ArrayStore reserved min(lhs.len(), rhs.len()), but a difference can hold lhs.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.
  2. The assert. This turns an under-reserved VecWriter from a silent reallocation into a panic, which is now stated on VecWriter::new. The other five call sites reserve a true upper bound (l + r for | and ^, min for &, l for -).

Existing pairwise_*/ref_ref benches, 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.

Apple M2 Max EPYC 7713 (Zen 3) EPYC 9845 (Zen 5)
& -0.8% [-1.3, -0.4] -6.8% [-7.6, -6.0] -7.0% [-8.0, -6.0]
| -1.6% [-2.3, -0.9] -22.6% [-23.2, -21.9] -22.8% [-23.1, -22.6]
^ -2.4% [-3.2, -1.6] -22.4% [-23.2, -21.6] -23.7% [-23.9, -23.6]
- -0.1% [-1.5, +1.3] -16.9% [-17.6, -16.2] -14.0% [-14.2, -13.9]
same binary twice, worst of the four -0.4% [-1.5, +0.7] -0.6% [-2.4, +1.1] +1.0% [+0.2, +1.8]

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_sub on 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 simd on 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:

  • An unchecked write behind an unsafe constructor. 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.
  • Per-operation constructors (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).

emschwartz and others added 2 commits October 11, 2026 04:17
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 branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant