perf(dv): serialize deletion vectors in one buffer

[fallintoplace]

Summary

• Writes the DV envelope directly into one bytes.Buffer.
• Reserves the exact portable bitmap size after run-length encoding.
• Patches the length field after the bitmap is written, then appends the CRC.
• Keeps the existing wire format and Java byte-for-byte fixtures unchanged.

Benchmark

Command:

go test ./table/dv -run ^$ -bench ^BenchmarkSerializeDV$ -benchmem -benchtime=200ms -count=5

Median of 5 runs on Apple M1 Pro, Go 1.25.9:

Workload	Before	After
sparse-1k	3,153 ns/op, 7,664 B/op, 11 allocs/op	2,018 ns/op, 2,512 B/op, 5 allocs/op
sparse-100k	227,145 ns/op, 855,235 B/op, 14 allocs/op	142,696 ns/op, 205,280 B/op, 5 allocs/op
sparse-1m	336,744 ns/op, 1,807,430 B/op, 14 allocs/op	171,052 ns/op, 508,482 B/op, 5 allocs/op

Checks

• go test ./table/dv -count=1 -timeout=5m
• go test ./table/dv -race -count=1 -timeout=5m
• go test ./table -count=1 -timeout=5m
• go test ./... -run ^$ -count=1
• go vet ./table/dv

[laskoviymishka]

This is a clean win: folding the two-allocation path into one pre-sized buffer is exactly right, and the envelope traces byte-for-byte against both the old code and the Java layout, so there's no wire-format or interop risk. The serializedSize() helper and its unit test are a nice way to keep the pre-size honest.

One blocking thing before merge: serializedSize() narrows GetSerializedSizeInBytes() from uint64 to int. On a 32-bit build a large bucket flips that negative and panics the buf.Grow call, and it's the exact pattern gosec's G115 catches. Keeping the helper in uint64 and converting once at the Grow call site clears both.

Everything else is optional: I left inline notes on closing the pre-existing uint32 length truncation while we're reshaping that code, a Java-fixture byte test to lock the wire format now that the assembly changed, and a couple of benchmark tweaks (it only exercises bucket 0, and the warmup already run-optimizes the bitmap before the timed loop).

Fix the narrowing and this is good to land, happy to take another pass.

[laskoviymishka]

This int(bm.GetSerializedSizeInBytes()) narrows a uint64 to int, and it's the one thing I'd want fixed before this lands.

On a 32-bit build any bucket serializing to ≥ 2 GiB flips negative, size goes negative, and the buf.Grow(...) call in SerializeDV panics rather than under-allocates. It's also the exact G115 pattern gosec flags, so it'll trip the linter if that rule's on.

I'd keep the whole helper in uint64 and convert once at the call site:

func (b *RoaringPositionBitmap) serializedSize() uint64 {
    size := uint64(8)
    for _, bm := range b.bitmaps {
        if bm.GetCardinality() > 0 {
            size += 4 + bm.GetSerializedSizeInBytes()
        }
    }
    return size
}

then guard the sum against math.MaxInt before the int conversion feeding buf.Grow.

[laskoviymishka]

This uint32(bitmapDataEnd-dvLengthSize) cast silently wraps if the bitmap ever serializes past 4 GiB: the length prefix goes out wrong and DeserializeDV later rejects the blob with a confusing length-mismatch instead of a clean "too large" at write time.

It's not new (the old uint32(innerLen) had the same gap) and it's a narrow path, but the rewrite is a natural spot to close it. I'd add a guard before the patch:

innerLen := bitmapDataEnd - dvLengthSize
if innerLen > math.MaxUint32 {
    return nil, fmt.Errorf("deletion vector payload too large: %d bytes", innerLen)
}

wdyt?

[laskoviymishka]

Not blocking: I traced the output and it's byte-identical to the old path. But since this rewrite reassembles the envelope (zero-fill + patch, magic via a fixed array), the wire format is now only guarded by our own round-trip.

I'd add one test that runs SerializeDV on the same input as TestDeserializeDV and asserts the bytes equal a Java-produced fixture (e.g. small-alternating-values-position-index.bin). That pins the interop contract so a future refactor can't drift the layout while still round-tripping cleanly through our own reader. wdyt?

[laskoviymishka]

Small thing: the size is only exact once the inner bitmaps are run-optimized, and SerializeDV guarantees that by calling RunLengthEncode() first. Worth a sentence here so a future caller doesn't invoke this pre-RLE and quietly under-size the Grow.

[laskoviymishka]

All three cases keep positions under 2^32, so everything lands in bucket 0 and the multi-bucket path in serializedSize() (the new map iteration) never gets exercised here.

Could we add a case that straddles the boundary, e.g. positions at 0 and 1<<32, so the pre-size hint gets validated across bucket-key gaps too?

[laskoviymishka]

Heads up that this warmup call mutates bitmap in place: SerializeDV runs RunLengthEncode() on its argument, so by the time the timed loop hits it the bitmap is already run-optimized and RunLengthEncode is a near-no-op every iteration.

If measuring the steady-state re-serialize path is the intent, a one-line comment saying so is enough; if you want the full encode-from-scratch cost, build a fresh bitmap inside the loop. Either's fine, just worth being explicit about which we're measuring.

[laskoviymishka]

👍