Repository navigation
perf: inlinable bit helpers, zeroOut skip, and accumulator window clamp - #55
Conversation
![orca-security-eu[bot]](https://avatars.githubusercontent.com/in/276244?s=60&v=4){kind=small}
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
 Passed |
Secrets |  0  0  0  0 |
View in Orca |
00d3bed to
d1a1c82
Compare
d1a1c82 to
cadbdf5
Compare
cadbdf5 to
9daeec8
Compare
9daeec8 to
561cc38
Compare
There was a problem hiding this comment.
Pull request overview
This PR applies three behavior-preserving performance optimizations to hot paths in the sroar bitmap library: replacing a lookup-table bit mask with inlinable helpers, skipping redundant clears when containers are already empty, and narrowing Accumulator build work to the touched-word window per staging block.
Changes:
- Replaced the
bitmapMasktable with an inlinablebitmapMask(pos)function plussetBit/clearBit/hasBithelpers, updating call sites across container/bitmap/accumulator code. - Optimized
zeroOutContainerto return early when the container is already empty, avoiding unnecessary 8KB clears for bitmap payloads. - Updated
Accumulator.layout/buildIntoto record and use a per-block touched-word window, reducing popcount/copy work for sparse clustered blocks; updated tests/benchmarks accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| container.go | Adds bit helpers, converts mask table to function, updates bit operations, and skips zeroOutContainer clear when already empty. |
| container_opt.go | Switches optimized container operations to use the new bit helpers and functional bitmapMask. |
| bitmap_test.go | Adds an invariant test for repeated ZeroOut() with follow-up merge to surface stale payload bits. |
| bitmap_opt.go | Updates bitmap fill/set-range paths to use the functional bitmapMask / setBit. |
| benchmark_accumulator_test.go | Adds a clustered “tiny” benchmark to measure the Accumulator touched-window optimization. |
| accumulator.go | Uses bit helpers and introduces blockLayout + touched-word window extraction for faster builds. |
| accumulator_test.go | Adds a regression test for interior touched-window extraction correctness. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // zeroOutContainer empties c. A zero cardinality header implies the payload | ||
| // is already zero — every writer that zeroes the header also zeroes the | ||
| // payload — so the 8KB clear is skipped then (And-style passes re-zero the | ||
| // same emptied containers repeatedly). |
There was a problem hiding this comment.
Fixed — the claim now scopes to bitmap payloads. Arrays do keep stale values past a zero cardinality, as zeroOutContainer itself does.
| // Splitting the prefix/suffix zero-scan from the popcount beats | ||
| // tracking the window inside the popcount loop, which would tax every | ||
| // word of every block. Each word is still visited only once. |
There was a problem hiding this comment.
Right — it's N+2 reads per non-empty block, since the two boundary words are read by both a scan and the popcount. Reworded to say that.
| // payload — so the 8KB clear is skipped then (And-style passes re-zero the | ||
| // same emptied containers repeatedly). | ||
| func zeroOutContainer(c []uint16) { | ||
| if isEmpty(c) { |
There was a problem hiding this comment.
issue: The isEmpty guard inside zeroOutContainer un-inlines (*Bitmap).ZeroOut (measured: inline cost 70 → 85, past the 80 budget), losing inlining at four And/AndNot fast-path call sites the base branch had — a real regression in a perf PR. Moving the guard to the three repeat-zeroing call sites instead was verified to restore cost 70 with tests green, and it also restores the old self-healing scrub for corrupt FromBuffer-adopted containers (the one correctness-class finding, which is defense-in-depth only).
There was a problem hiding this comment.
Confirmed: cost 85 against the 80 budget, and the four And-family fast paths lose inlining. Fixed by scoping the guard to bitmaps and writing the test out instead of calling isEmpty — that's 15 cheaper and puts ZeroOut at exactly 80, so the skip and the inlining both survive; noted in the comment that there's no headroom left.
There was a problem hiding this comment.
suggestion: buildInto's bitmap branch ignores the window it just computed — it copies the full 8KB block where the windowed copy is strictly no worse and pays off exactly on the dense-clustered shape the PR's benchmark motivates (the added fixture never produces bitmap-typed blocks, so this shape is unmeasured).
There was a problem hiding this comment.
Taken, with clears either side of the window: for non-zero keys the destination comes from newContainerNoClr, so the full copy was doubling as the scrub for dirty pooled memory. Windowed is ~60% faster at the clustered window this shape actually produces (188 of 4096 words) and a wash at full width — regression test added, since the natural one passes even with the clears removed.
| // is already zero — every writer that zeroes the header also zeroes the | ||
| // payload — so the 8KB clear is skipped then (And-style passes re-zero the | ||
| // same emptied containers repeatedly). | ||
| func zeroOutContainer(c []uint16) { |
There was a problem hiding this comment.
issue: The new early return can leave an imported bitmap non-empty. FromBufferWithCopy accepts a zero-cardinality header with a nonzero bitmap payload; after ZeroOut and Set(1), HEAD reports cardinality 1 while Contains(0) and Contains(1) are true and ToArray is [0]. Could we either keep clearing bitmap payloads here or validate and canonicalize every FromBuffer input, then add this exported-API sequence as a regression test?
There was a problem hiding this comment.
Reproduced, but I couldn't find an in-library producer: no normal operation yields a zero-cardinality bitmap header over a nonzero payload, and neither does container creation over dirty memory — newContainer clears, scootRight zeroes the gap it opens, and expandContainer fully overwrites on the array→bitmap conversion. It needs a buffer sroar didn't write, and FromBuffer validates no other header field either, so I'd rather not special-case this one.
f15b81a to
9e2eb77
Compare
9e2eb77 to
d2d7cff
Compare
d2d7cff to
39cb721
Compare
bitmapMask becomes a function (0x8000 >> pos) — one shift instead of a table load plus bounds check, ~40% faster on the isolated deposit loop — and setBit/clearBit/hasBit compose it for the value-indexed sites, so deposit and merge loops state their intent instead of open-coding index/mask arithmetic. Fixed-word position loops (removeRange, rank, selectAt) and the uint64 mask composition keep bare bitmapMask(pos); orArray's popcount-diff counting branch keeps its branchless form. bitmap.add sat at exactly the inlining budget (cost 80) and the helper calls pushed it over, un-inlining it from Set (+2.25% measured); hoisting the mask into a local restores it (cost 77). Interleaved A/B benchmarks against the previous commit are statistically flat (geomean +0.03%), allocations identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A zero cardinality header implies the payload is zero — every writer that zeroes the header zeroes the payload with it — so the unconditional 8KB memset is wasted on containers that are already empty. And-style passes (AndConc under repeated ContainsAll folds, ZeroOut on pooled bitmaps) re-zero the same emptied containers every pass: measured 5.9µs -> 146ns per ZeroOut over 100 already-empty bitmap containers. Non-empty paths are unaffected (interleaved A/B flat). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
layout previously popcounted all 4096 words of every staging block and
buildInto walked them all again, so finalize cost tracked blocks, not
content — for clustered doc-ID runs ~98% of both passes covered provably
zero words. layout now records each block's touched window alongside its
cardinality (blockLayout{card,min,max} replaces the cards slice at the
same 8 bytes per entry) and buildInto extracts array containers from that
window only, via an offset-aware bitsIntoArray. Dense copies stay whole:
blocks that finalize as bitmap containers have near-full windows anyway.
The scan visits every word exactly once — zero-test the untouched prefix
and suffix in from both ends, popcount only the window between — a shape
suggested by Andrzej after in-loop min/max tracking measured +3.4% on
dense blocks. Interleaved A/B vs the previous commit: -46% on clustered
sequential unions, -5.5% uniform sparse, flat on dense and few-block
shapes, allocations identical.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…copy The isEmpty guard in zeroOutContainer put ZeroOut at inline cost 85 against a budget of 80, so the And/AndConc/AndMasked/AndMaskedConc fast paths stopped inlining it — a regression in a perf change. Scoping the guard to bitmaps, where the clear is, and writing the test out instead of calling isEmpty costs 15 less and lands ZeroOut on exactly 80, keeping both the skip and the inlining. buildInto's bitmap branch computed a touched-word window and then copied the whole 8KB block. It now copies just the window and clears either side, which is ~60% faster on the clustered blocks this shape produces and a wash when the window spans the block. The clears are required, not incidental: for non-zero keys the container comes from newContainerNoClr, so the full copy had been doubling as the scrub for dirty pooled memory. The added test places values mid-block and inspects the payload, since a build that skips the clears still passes a ToArray comparison. Also corrects two comments: a zero cardinality implies a zeroed payload only for bitmaps, and the layout scan reads each block's two boundary words twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
39cb721 to
9c7ca5a
Compare
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Passed |
Infrastructure as Code | 0 0 0 0 |
View in Orca |
| Passed |
SAST | 0 0 0 0 |
View in Orca |
| Passed |
Secrets | 0 0 0 0 |
View in Orca |
| Passed |
Vulnerabilities | 0 0 0 0 |
View in Orca |
Motivation
Three independent, behavior-preserving optimizations to hot paths in the sroar bitmap library: per-bit index/mask arithmetic, the unconditional 8KB clear in
zeroOutContainer, and the Accumulator's whole-block popcount-and-copy passes. No exported API or observable behavior changes.Approach
Bit helpers replace the
bitmapMasktable.bitmapMaskbecomes a function (0x8000 >> pos) — one shift instead of a table load plus bounds check. NewsetBit/clearBit/hasBitcompose it, replacing ~10 open-codedindex/masksites acrosscontainer*.go,accumulator.go,bitmap_opt.go. Fixed-word-position loops and uint64 composition keep barebitmapMask(pos).bitmap.addsat at the inlining budget, so the mask is hoisted to a local to keep it inlined intoSet.zeroOutContainerskips the clear when already empty. A zero-cardinality header implies zero payload — every writer that zeroes the header zeroes the payload — so the memset is wasted on empty containers. And-style folds that re-zero the same emptied containers each pass: 5.9µs → 146ns perZeroOutover 100 empty containers. Non-empty paths unchanged.Accumulator build clamps to each block's touched-word window.
layoutpreviously popcounted all 4096 words of every staging block andbuildIntowalked them again; for clustered doc-ID runs ~98% covered provably zero words.layoutnow records each block's touched window (blockLayout{card,min,max}, same 8 bytes as the oldcardsslice), andbuildIntoextracts array containers from that window only via an offset-awarebitsIntoArray. Dense blocks keep whole copies.Key areas for review
container*.go— the helper substitution is mechanical; confirm the sites kept on barebitmapMask(pos)are position-indexed, not value-indexed.zeroOutContainer— the skip rests on "card 0 ⇒ payload 0", already load-bearing for reads (all()/ToArrayignore the header); writers were traced.accumulator.go:layout/buildInto— prefix/suffix zero-scan split from the popcount (each word visited once);bitsIntoArray's newbaseWordoffset stays overrun-safe against an undersized output.Testing
New equivalence guards (
bitmapMaskround-trips, set/clear/has, interior window extraction, repeated-zeroOutinvariant) extend the existing tables. Full suite green under-race;go vet/golangci-lintclean on changed lines. Interleaved A/B: bit helpers geomean flat (+0.03%), accumulator -46% clustered / -5.5% uniform sparse / flat on dense. Allocations identical across all three.