From d1d76d9d1ccdc226b8bb932634da8fcc0596cea2 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Sat, 8 Aug 2026 16:03:03 -0600 Subject: [PATCH 1/3] perf: unpack bit-packed values a word at a time, not a bit at a time (#501) 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. It was the largest single frame in a CPU profile of both scan shapes: 21.0% of a filtered aggregate and 19.2% of a row-returning projection, against 4-7% for the decode machinery around it. A value of `width` bits at bit offset `shift` spans at most 64 + 7 bits, so one unaligned 64-bit load plus at most one further byte covers it. Both are little-endian reads, which this format already requires (spec 3). The high-byte shift-in is guarded by `shift + width > 64`, which cannot hold when shift is 0 because width is at most 64 -- so `64 - shift` is never a shift by 64. The fast path runs only where its nine-byte window lies inside the encoded body; the tail keeps the per-bit assembly rather than over-reading. The over-read would be at most seven bytes and would usually land inside the same allocation, which is the kind of defect that passes every test and then fails under a sanitizer. Proven byte-identical against a kept reference, in the shape this file already uses for bitpack: ref_bitunpack is the pre-#501 per-bit loop verbatim, and 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. That is 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 of the rewrite rather than by deleting the check: with the high-byte carry removed, so that only values spanning past 64 bits are wrong, FAIL bitunpack width=59 n=7 value 5: 34962621760722854 vs reference 179077809836578726 which is exactly the case the guard exists for. Refs #501 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FeNm2Gw6h16Z123We3F1vJ --- src/columnar_encoding.c | 127 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 118 insertions(+), 9 deletions(-) diff --git a/src/columnar_encoding.c b/src/columnar_encoding.c index 490c0f15..0ed885de 100644 --- a/src/columnar_encoding.c +++ b/src/columnar_encoding.c @@ -185,19 +185,63 @@ bitunpack(const unsigned char *in, uint32 inLen, uint32 n, int width, if (((uint64) n * (uint32) width + 7) / 8 > inLen) DECODE_CORRUPT("bit-packed body exceeds encoded length"); - for (i = 0; i < n; i++) + /* + * Word at a time (#501). The per-bit loop this replaces ran once per BIT -- + * a load, two shifts, a mask, a test and an or for each one -- and was the + * largest single frame in a profile of both scan shapes, at 21.0% of a + * filtered aggregate and 19.2% of a row-returning projection. + * + * A value of `width` bits starting at bit offset `shift` occupies at most + * 64 + 7 bits, so one unaligned 64-bit load plus at most one more byte + * covers it. Both are little-endian reads, which this format already + * requires (spec 3), and the shift-in of the high byte is guarded: it runs + * only when shift + width > 64, which cannot hold with shift == 0 because + * width is at most 64, so `64 - shift` is never a shift by 64. + * + * The fast path is used only where its 9-byte window lies inside the buffer. + * The tail falls back to the reference loop rather than reading past the end + * -- the over-read would be at most seven bytes and would usually land in the + * same allocation, which is exactly the kind of bug that survives every test + * and fails under a sanitizer on someone else's machine. + */ { - uint64 v = 0; - int b; + const uint64 mask = (width == 64) + ? ~UINT64CONST(0) : ((UINT64CONST(1) << width) - 1); + uint32 nbytes = (uint32) (((uint64) n * (uint32) width + 7) / 8); - COLUMNAR_DECODE_INTERRUPT(i); - for (b = 0; b < width; b++) + for (i = 0; i < n; i++) { - if ((in[(bitpos + b) >> 3] >> ((bitpos + b) & 7)) & 1) - v |= (uint64) 1 << b; + uint64 bytepos = bitpos >> 3; + unsigned shift = (unsigned) (bitpos & 7); + uint64 v; + + COLUMNAR_DECODE_INTERRUPT(i); + + if (bytepos + 9 <= nbytes) + { + uint64 lo; + + memcpy(&lo, in + bytepos, sizeof(uint64)); + v = lo >> shift; + if (shift + (unsigned) width > 64) + v |= (uint64) in[bytepos + 8] << (64 - shift); + } + else + { + /* tail: assemble only the bytes that exist */ + int b; + + v = 0; + for (b = 0; b < width; b++) + { + if ((in[(bitpos + b) >> 3] >> ((bitpos + b) & 7)) & 1) + v |= (uint64) 1 << b; + } + } + + out[i] = v & mask; + bitpos += width; } - out[i] = v; - bitpos += width; } } @@ -2532,6 +2576,42 @@ PgColumnarDecodeChunk(const char *enc, uint32 encLen, int encodingType, * catalog, like the other debug hooks. * ------------------------------------------------------------------------- */ +/* + * Reference bit unpacker: the pre-#501 per-bit loop, verbatim. + * + * The oracle for the word-at-a-time reader that replaced it. Kept naive on + * purpose -- it walks one bit at a time and that is the whole point, because the + * claim the rewrite has to make is that it is byte-identical, and an oracle that + * shares the fast path's cleverness cannot check that. + */ +static void +ref_bitunpack(const unsigned char *in, uint32 n, int width, uint64 *out) +{ + uint64 bitpos = 0; + uint32 i; + + if (width == 0) + { + for (i = 0; i < n; i++) + out[i] = 0; + return; + } + + for (i = 0; i < n; i++) + { + uint64 v = 0; + int b; + + for (b = 0; b < width; b++) + { + if ((in[(bitpos + b) >> 3] >> ((bitpos + b) & 7)) & 1) + v |= (uint64) 1 << b; + } + out[i] = v; + bitpos += width; + } +} + /* Reference bit packer: the pre-#285 per-bit loop. */ static void ref_bitpack(const uint64 *vals, uint32 n, int width, StringInfo out) @@ -2696,6 +2776,35 @@ pgcolumnar_debug_encoding_selftest(PG_FUNCTION_ARGS) ref_bitpack(vals, n, width, &b); cases++; + /* + * And the reader (#501). The word-at-a-time unpacker must agree with + * the per-bit reference value for value, at every width and at every + * sub-byte start offset the counts above produce. This is the only + * place widths above ~32, the nine-byte field span and the width==64 + * mask are reached at all. + */ + { + uint64 *got = palloc(sizeof(uint64) * n); + uint64 *want = palloc(sizeof(uint64) * n); + uint32 j; + + bitunpack((const unsigned char *) a.data, (uint32) a.len, + n, width, got); + ref_bitunpack((const unsigned char *) a.data, n, width, want); + cases++; + + for (j = 0; j < n; j++) + { + if (got[j] != want[j]) + SELFTEST_FAIL( + "bitunpack width=%d n=%u value %u: " UINT64_FORMAT + " vs reference " UINT64_FORMAT, + width, n, j, got[j], want[j]); + } + pfree(got); + pfree(want); + } + if (a.len != b.len) SELFTEST_FAIL("bitpack width=%d n=%u: length %d vs reference %d", width, n, a.len, b.len); From fd2caa000b4e44f814bdc771ad7d697cda782ef7 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Sat, 8 Aug 2026 16:07:05 -0600 Subject: [PATCH 2/3] perf: hoist the bounds test out of the unpack loop (#501) A per-value 'does the nine-byte window fit' test cost more than the entire per-bit loop it replaced when width was 1: measured 1.7% slower on a monotonic bigint, where delta encoding packs to a single bit and the old inner loop ran one iteration. The count of 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. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FeNm2Gw6h16Z123We3F1vJ --- src/columnar_encoding.c | 61 ++++++++++++++++++++++++++--------------- 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/src/columnar_encoding.c b/src/columnar_encoding.c index 0ed885de..030713f1 100644 --- a/src/columnar_encoding.c +++ b/src/columnar_encoding.c @@ -209,37 +209,54 @@ bitunpack(const unsigned char *in, uint32 inLen, uint32 n, int width, ? ~UINT64CONST(0) : ((UINT64CONST(1) << width) - 1); uint32 nbytes = (uint32) (((uint64) n * (uint32) width + 7) / 8); - for (i = 0; i < n; i++) + /* + * How many values the wide load can serve, computed once rather than + * tested per value. Value i starts at byte (i * width) / 8 and needs + * nine bytes, so the condition is i * width <= 8 * (nbytes - 9). + * + * Hoisting this matters at small widths: a per-value bounds test cost + * more than the whole per-bit loop it was replacing when width was 1, + * measured at 1.7% slower on a monotonic bigint before the split. + */ + uint32 nFast = 0; + + if (nbytes >= 9) + { + uint64 maxIdx = (8 * (uint64) (nbytes - 9)) / (uint32) width; + + nFast = (maxIdx + 1 < (uint64) n) ? (uint32) (maxIdx + 1) : n; + } + + for (i = 0; i < nFast; i++) { - uint64 bytepos = bitpos >> 3; + uint64 lo; unsigned shift = (unsigned) (bitpos & 7); uint64 v; COLUMNAR_DECODE_INTERRUPT(i); - if (bytepos + 9 <= nbytes) - { - uint64 lo; + memcpy(&lo, in + (bitpos >> 3), sizeof(uint64)); + v = lo >> shift; + if (shift + (unsigned) width > 64) + v |= (uint64) in[(bitpos >> 3) + 8] << (64 - shift); - memcpy(&lo, in + bytepos, sizeof(uint64)); - v = lo >> shift; - if (shift + (unsigned) width > 64) - v |= (uint64) in[bytepos + 8] << (64 - shift); - } - else - { - /* tail: assemble only the bytes that exist */ - int b; + out[i] = v & mask; + bitpos += width; + } - v = 0; - for (b = 0; b < width; b++) - { - if ((in[(bitpos + b) >> 3] >> ((bitpos + b) & 7)) & 1) - v |= (uint64) 1 << b; - } - } + /* Tail: assemble only the bytes that exist, one bit at a time. */ + for (; i < n; i++) + { + uint64 v = 0; + int b; - out[i] = v & mask; + COLUMNAR_DECODE_INTERRUPT(i); + for (b = 0; b < width; b++) + { + if ((in[(bitpos + b) >> 3] >> ((bitpos + b) & 7)) & 1) + v |= (uint64) 1 << b; + } + out[i] = v; bitpos += width; } } From cd4ddd92fbe0e5276ed273572148296b9de89a82 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Sat, 8 Aug 2026 16:31:06 -0600 Subject: [PATCH 3/3] test: derive the count that reaches the wide path, do not rely on the list (#501) Raised in review of #514. Coverage of the unpacker's two paths is a property of the counts array, not of the loop: nFast is zero until the encoded body reaches nine bytes, which needs n * width >= 65. A shorter list would exercise low widths through the TAIL only -- and the tail is the per-bit assembly, which is exactly what ref_bitunpack does, so the oracle would be comparing the reference against itself and passing. 'Every width from 1 to 64' would still read as complete coverage. The fixed list is sufficient today (verified: every width reaches the fast path, and every width exercises both paths in one call -- width 1 with n=129 splits 65 fast and 64 tail). But that is a fact about nine constants, and trimming them would narrow the oracle silently. So one count per width is now derived from the width rather than listed, and checked, so it 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 Also notes at the tail that it is deliberately bit-indexed and endian- independent where the fast path is not. 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, and that is what 'simplifying' the tail into the same word trick would produce. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FeNm2Gw6h16Z123We3F1vJ --- src/columnar_encoding.c | 41 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/src/columnar_encoding.c b/src/columnar_encoding.c index 030713f1..9fbe442d 100644 --- a/src/columnar_encoding.c +++ b/src/columnar_encoding.c @@ -244,7 +244,17 @@ bitunpack(const unsigned char *in, uint32 inLen, uint32 n, int width, bitpos += width; } - /* Tail: assemble only the bytes that exist, one bit at a time. */ + /* + * Tail: assemble only the bytes that exist, one bit at a time. + * + * Deliberately bit-indexed and therefore endian-independent, where the + * fast path above is not. Spec 3 already requires a little-endian host, + * so this is not a second contract -- but do not "simplify" this into + * the same word trick. The fast path is bounded by nFast precisely + * because the wide load would read past the encoded body here, and an + * array that is correct above nFast and wrong below it is far nastier to + * diagnose than one that is uniformly wrong. + */ for (; i < n; i++) { uint64 v = 0; @@ -2768,12 +2778,39 @@ pgcolumnar_debug_encoding_selftest(PG_FUNCTION_ARGS) for (width = 1; width <= 64; width++) { static const uint32 counts[] = {1, 2, 3, 7, 8, 9, 17, 64, 129}; + uint32 ns[lengthof(counts) + 1]; uint64 state = UINT64CONST(0x155) + (uint64) width; uint32 ci; + /* + * One derived count per width, in addition to the fixed list (#514 + * review). Coverage of the unpacker's two paths is a property of these + * counts and not of the loop: nFast is zero until the encoded body + * reaches nine bytes, which needs n * width >= 65, so a shorter list + * would exercise low widths through the TAIL only -- and the tail is the + * per-bit assembly, which is what ref_bitunpack does. The oracle would + * then be comparing the reference against itself and passing. + * + * Derived rather than listed so that trimming counts[] above cannot + * silently narrow this, and checked below so it cannot rot either. + */ + ns[lengthof(counts)] = (65 + (uint32) width - 1) / (uint32) width + 4; for (ci = 0; ci < lengthof(counts); ci++) + ns[ci] = counts[ci]; + + { + uint32 dn = ns[lengthof(counts)]; + uint32 dbytes = (uint32) (((uint64) dn * (uint32) width + 7) / 8); + + if (dbytes < 9) + SELFTEST_FAIL("width=%d: derived n=%u yields %u bytes, " + "which never reaches the wide load", + width, dn, dbytes); + } + + for (ci = 0; ci < lengthof(ns); ci++) { - uint32 n = counts[ci]; + uint32 n = ns[ci]; uint64 *vals = palloc(sizeof(uint64) * n); StringInfoData a; StringInfoData b;