perf: unpack bit-packed values a word at a time, not a bit at a time (#501)

[jdatcmd]

bitunpack ran its inner loop once per bit — a byte load, two shifts, a mask, a test and an or for every bit of every value. This reads one unaligned 64-bit word instead, and is byte-identical.

What the win actually is, which is not what the issue first claimed

The saving scales with bit width, because the per-bit loop runs width iterations per value and the word path runs one. @ChronicallyJD has since amended #501's title accordingly.

A/B on the bench, same cluster, same data, only the .so differing, postmaster restarted between arms, medians of 3, answers identical throughout:

shape	main	this branch
sum(a) — bigint, delta-packed to ~1 bit	155.2 ms	154.4 ms	no change
count(*) WHERE b = 7 — int, width 10	128.7 ms	111.2 ms	13.6% faster
per-row materialisation	277.0 ms	272.9 ms	~1.5%

On ClickBench there is no measurable difference at all. The queries I tried first (equality on ClientTimeZone, GROUP BY CounterID, sum(ResolutionWidth)) either prune to zero rows or are not dominated by unpacking. This is a primitive-level win that shows up where unpacking dominates; it should not be quoted as a workload number.

The mechanism, confirmed by profile rather than asserted

cpu-clock, 999 Hz, 5K samples per shape, distinct capture files, on the same fixture:

width ~1   sum(a)            bitunpack: not in the top 5 frames
width 10   count WHERE b=7   bitunpack: 17.22%   (second-largest frame)

That is a sharper claim than any single percentage: the share tracks width, which is what the mechanism predicts and what the A/B shows.

Byte-identical, proven the way this file already proves bitpack

ref_bitunpack is the pre-#501 per-bit loop kept verbatim as the oracle, beside the existing ref_bitpack. The encoding selftest now compares the two value by value at every width from 1 to 64, at every sub-byte start offset the existing value counts produce — the only place widths above ~32, the nine-byte field span and the width == 64 mask are reached at all, since the encoder never selects packing there.

The check has teeth, proven by breaking the subtlest part rather than by deleting the check. With the high-byte carry removed, so that only values spanning past 64 bits go wrong:

FAIL bitunpack width=59 n=7 value 5:
     34962621760722854 vs reference 179077809836578726

A coarser break would have failed at width 1 and told me nothing about the case the guard exists for.

Second commit: the bounds test had to come out of the loop

The first version was 1.7% slower at width 1 — a per-value "does the nine-byte window fit" test cost more than the entire per-bit loop it replaced, when that loop ran one iteration. How many values the wide load can serve is arithmetic on nbytes and width, so it is computed once and the loop splits into a fast body with no bounds test and a per-bit tail. That removed the regression and kept the width-10 win.

The tail is deliberate: the fast path runs only where its nine-byte window lies inside the encoded body. An over-read would be at most seven bytes and would usually land in the same allocation — the kind of defect that passes every test and then fails under a sanitizer.

Gate

Five majors, all green, at this head:

PASS PG15 (129 ran, 5 skipped) · PASS PG16 (129) · PASS PG17 (129)
PASS PG18 (132 ran, 2 skipped) · PASS PG19 (134 ran, 0 skipped)
ALL VERSIONS PASSED · exit=0

encode_invariants=PASS and native_encoding=PASS on all five, and neither is in any skip list — a green major and "the suite that checks this ran" are different claims.

Refs #501

[ChronicallyJD]

Read the arithmetic rather than the description, and checked the one thing that
would make the oracle vacuous.

The bounds are conservative in the right direction. The necessary condition is
floor(i*width/8) <= nbytes - 9; yours is i*width <= 8*(nbytes-9), which is
strictly stronger, so values near the boundary fall to the tail rather than off
the end. nbytes >= 9 guards the unsigned subtraction. That is the right way to
be wrong about a bounds test.

The high-byte shift is sound and the guard is exact. shift + width > 64
cannot hold with shift == 0 because width <= 64, so 64 - shift lies in
[57,63] and is never a shift by 64. One extra byte is also sufficient rather than
merely convenient: the residual is shift + width - 64, maximised at 7 when
shift == 7 and width == 64, and a byte supplies 8.

width == 64 special-casing the mask avoids the 1 << 64 UB, and
COLUMNAR_DECODE_INTERRUPT survives in both loops.

The thing I checked because the oracle could have been vacuous

An exhaustive comparison at every width proves nothing about the fast path if the
fixture never reaches it — nFast > 0 needs n * width >= 72, so a small
counts[] would have tested widths 1..9 through the tail only, which is the
reference implementation compared against itself.

counts[] = {1, 2, 3, 7, 8, 9, 17, 64, 129} is sufficient: every width from 1 to
64 has at least one count reaching the fast path, and the interesting case is
width 1 with n=129, where nFast is 65 — values 0..64 take the wide load and
65..128 take the tail, in the same call. Both paths, one comparison, every width.

Worth stating in the PR because "at every width from 1 to 64" reads as complete
coverage and is only complete given those counts; a later trim of that array would
silently narrow it to the tail at small widths.

One note, not a blocker: the two paths disagree on a big-endian host

The tail assembles values by indexing bits, so it is endian-independent. The fast
path is memcpy into a uint64 and shift, which is little-endian by
construction. Your comment cites spec 3, and that is the right answer — the format
already stores host-endian integers and already assumes little-endian, so this
introduces no portability break that was not there.

What is new is the shape of the break. Before, bitunpack was one of the few
pieces that would have produced correct values on a big-endian machine. Now the
same call would return wrong values for indices below nFast and correct ones
above it — an array correct at one end and wrong at the other, which is a nastier
thing to diagnose than uniformly wrong. Worth one line at the tail loop saying it
is endian-independent and the fast path is not, so nobody later "simplifies" the
tail into the same word trick believing that restores consistency.

And on the framing

Carrying "no measurable difference on ClickBench" into the body beside the 13.6%
is the right call and it is my error you are guarding against — I filed this issue
claiming ~20% of every scan from two shapes of one fixture. The profile showing
the share tracking width (absent from the top five at ~1 bit, 17.22% at width
10) is a better argument than any single percentage, because it is a prediction
the mechanism makes rather than a number a fixture happened to produce.

Approving.

[jdatcmd]

Both review points taken, cd4ddd9.

The oracle could have been vacuous, and now cannot be trimmed into vacuity

Your catch, verified independently before acting on it. nFast is zero until the encoded body reaches nine bytes — exactly n * width >= 65 — so a shorter counts[] would have exercised low widths through the tail only. The tail is the per-bit assembly, which is what ref_bitunpack is, so the oracle would have been comparing the reference against itself and passing, while "every width from 1 to 64" still read as complete coverage.

The current list is sufficient. I checked all 64 widths rather than spot-checking:

widths lacking fast-path coverage:        none
widths never exercising both in one call: none
width 1, n=129: nbytes=17, nFast=65   (65 fast, 64 tail — your figure exactly)

But that is a fact about nine constants. So one count per width is now derived from the width instead of listed, and checked, so trimming the array cannot narrow the oracle and the derivation cannot rot either. Proven by breaking the derivation rather than deleting the check:

FAIL width=1: derived n=1 yields 1 bytes, which never reaches the wide load

The tail's endian-independence is now stated

Your reading is right and the shape of the risk is the part worth writing down: spec 3 already requires a little-endian host, so this is not a second contract — but an array correct above nFast and wrong below it is far nastier to diagnose than one uniformly wrong. That is precisely what "simplifying" the tail into the same word trick would produce, so the tail now says it is deliberately bit-indexed and why.

Re-verified

encode_invariants on all five majors at this head, built clean per major with rm -f src/*.o between: 11 checks, 0 fails on each. The change is to the selftest and one comment, so I did not re-run the full matrix — the five-major green at fd2caa0 still describes the shipped code path. Say the word if you want the matrix re-run on principle.

One small correction to the arithmetic in your note, for the record rather than because it changes anything: the threshold is n * width >= 65, not 72. 72 is the bit count of nine bytes; the +7 in (n*width+7)/8 means 65 bits already round up to nine.