Skip to content

Fix BatchEqual matching after carries beyond 64 bit planes - #586

Open
perfloop-agent wants to merge 2 commits into
RoaringBitmap:masterfrom
perfloop-oss:perfloop-pr-open-mwfw5c14bs
Open

perfloop-agent wants to merge 2 commits into
RoaringBitmap:masterfrom
perfloop-oss:perfloop-pr-open-mwfw5c14bs

Conversation

@perfloop-agent

Copy link
Copy Markdown
Contributor

Description

Fix a regression introduced by our perfloop-agent contribution in #532, merged as dac837d3bbc8. Its BatchEqual bit-plane trie can miss matching columns once arithmetic grows a BSI beyond 64 planes. At p=64, the runtime mask uint64(1) << uint(p) is zero, so the walk incorrectly requires plane 64 to be clear.

b := NewBSI(Max64BitSigned, Min64BitSigned)
b.SetValue(1, -1)
b.IncrementAll()

This produces BitCount() == 65 and GetValue(1) == (0, true). For BatchEqual(0, []int64{0}).ToArray(), the #532 parent bd4128b returns [1], while the #532 merge and the current base return []. This fix restores [1]. Adding an index containing column 1 with value 1 produces the same mismatch.

The bug is in the trie path. The large-query scan fallback added by #533 uses GetValue and was already correct.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Changes Made

What was changed?

  • Limit the effective query width to 64 planes and add arithmetic-carry regression tests.

Why was it changed?

  • Carry planes above bit 63 cannot contribute to GetValue's int64 result.

How was it changed?

  • Cap the local bit count before the existing trie walk; retain stored planes, arithmetic, existence-bitmap authority and the scan fallback.

Testing

The Go test runner passed the BatchEqual tests in BitSliceIndexing, including TestBatchEqualCarry and TestBatchEqualSignedBoundaryCarries; both fail without the fix. They cover the IncrementAll and Add routes and deterministic differential membership near both signed limits through 65- and 66-plane states.

Formatting

The changed Go files are gofmt-clean.

Fuzzing

Not run. The template's FuzzSmat target is not defined in the repository, and smat exercises the root roaring package, not BitSliceIndexing.

Performance Impact

The existing benchmarks in BitSliceIndexing/bsi_benchmark_test.go are regression controls, not speedup claims. Both arms include the new tests: the baseline is this PR's head with the bsi.go change reverted. This keeps the compiled test binaries alike, because adding tests alone changes code layout. An initial comparison against the original test binary measured higher M200 time.

Local run with Go 1.24.4: three measured pairs per benchmark after an excluded warmup, randomized arm order, fresh processes, two-second samples and GOMAXPROCS=4. Benchmark setup stays outside the timed loop. Before/after medians and median paired changes (after minus before):

Benchmark ns/op: before → after (paired Δ) B/op: before → after (paired Δ) allocs/op: before → after (paired Δ)
BenchmarkBatchEqual 59473166 → 58742009 (-731157) 26735585 → 26735580 (-5) 8248 → 8248 (+0)
BenchmarkBatchEqualM128 2100422 → 2091976 (-14191) 9343343 → 9343343 (+0) 3034 → 3034 (+0)
BenchmarkBatchEqualM128Scattered 1694716244 → 1716678572 (+6329117) 157465452 → 156326580 (-1134484) 23903 → 23624 (-278)
BenchmarkBatchEqualM200 5561024 → 5618662 (+63336) 9566338 → 9566337 (+0) 7468 → 7468 (+0)
BenchmarkBatchEqualRunOptimizedEndToEnd 290.0 → 292.6 (+2.6) 176 → 176 (+0) 8 → 8 (+0)

Both increases and decreases are descriptive local estimates, not evidence of a speedup or zero overhead. These controls use normal indexes; carried-index frequency in applications remains unknown.

Local native output from the first measured pair, before then after
Before, then after: BenchmarkBatchEqual
BenchmarkBatchEqual-4   	      36	  58983341 ns/op	26735583 B/op	    8248 allocs/op
BenchmarkBatchEqual-4   	      40	  57889753 ns/op	26735578 B/op	    8248 allocs/op
Before, then after: BenchmarkBatchEqualM128
BenchmarkBatchEqualM128-4   	    1072	   2100422 ns/op	 9343343 B/op	    3034 allocs/op
BenchmarkBatchEqualM128-4   	    1093	   2080925 ns/op	 9343342 B/op	    3034 allocs/op
Before, then after: BenchmarkBatchEqualM128Scattered
BenchmarkBatchEqualM128Scattered-4   	       2	1694716244 ns/op	158599120 B/op	   24176 allocs/op
BenchmarkBatchEqualM128Scattered-4   	       2	1687668801 ns/op	157464636 B/op	   23900 allocs/op
Before, then after: BenchmarkBatchEqualM200
BenchmarkBatchEqualM200-4   	     417	   5555326 ns/op	 9566337 B/op	    7468 allocs/op
BenchmarkBatchEqualM200-4   	     423	   5618662 ns/op	 9566337 B/op	    7468 allocs/op
Before, then after: BenchmarkBatchEqualRunOptimizedEndToEnd
BenchmarkBatchEqualRunOptimizedEndToEnd-4   	 8223720	       290.0 ns/op	     176 B/op	       8 allocs/op
BenchmarkBatchEqualRunOptimizedEndToEnd-4   	 8170075	       292.6 ns/op	     176 B/op	       8 allocs/op

With Go 1.24.4, run each benchmark above on the test-only baseline and this revision using its exact name in the selection:

go test -buildvcs=false -c -o bsi.test ./BitSliceIndexing
cd BitSliceIndexing
../bsi.test -test.run='^$' -test.bench='^BenchmarkBatchEqual$' -test.benchtime=2s -test.benchmem -test.count=1 -test.cpu=4

Breaking Changes

None. Values retain GetValue's low-64-bit interpretation; arithmetic storage and CompareValue are unchanged.

Case and evidence


Made by Perfloop. Human sponsor: Tomás Senart. To ask for a change, comment or review in plain words. How Perfloop handles pull requests.

Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Tomás Senart <ts@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Tomás Senart <ts@perfloop.ai>
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