Conversation
Implements the PFOR (Patched Frame of Reference) integer compression algorithm as a standalone utility library in arrow/util/pfor/. Includes: - Cost model for optimal bit width selection (histogram-based) - Vector-level encode/decode with FOR + bit-packing + exceptions - Page-level wrapper with header, offset array, and multi-vector layout - Comprehensive unit tests covering edge cases and round-trips
Adds PFOR = 11 to the Encoding enum and wires it into the parquet read/write pipeline: - PforEncoder<DType> in encoder.cc (buffers values, calls PforWrapper::Encode) - PforDecoder<DType> in decoder.cc (decodes all values on first access) - PFOR case in column_reader.cc InitializeDataDecoder - Encoding string mapping in types.cc Supports INT32 and INT64 column types.
Benchmarks encode/decode throughput for int32/int64 across 10 data distributions inspired by Snowflake's NumericComprBenchmark: constant, sequential, small range, high-base-small-range (timestamps), with outliers (exception path), random, TPC-DS date/store/item/quantity keys. Each distribution runs at 1K/10K/100K/1M elements. Reports bytes/s, items/s, and compression ratio.
Load() now returns Result<PforVectorInfo> after the Status/Result refactoring. Use ASSERT_OK_AND_ASSIGN to properly unwrap the result in tests.
Make LoadHeader fallible: move the header-size check from Decode into LoadHeader, return Result<PforHeader>, and update Decode to use ARROW_ASSIGN_OR_RAISE. Mirrors the corresponding ALP review fix on gh540-alp-pseudoDecimal-encoding.
Replace std::memcpy / raw byte writes in PforWrapper::StoreHeader, LoadHeader, and the offset-array read/write paths with util::SafeLoadAs and util::SafeStore. Mirrors the corresponding ALP review fix on gh540-alp-pseudoDecimal-encoding.
Reject invalid packing_mode, value_byte_width mismatch, log_vector_size out of [kMin, kMax] range, and negative num_elements when loading the PFOR page header. Removes the redundant packing_mode and value_byte_width checks from Decode now that they live in LoadHeader. Mirrors the corresponding ALP review fix on gh540-alp-pseudoDecimal-encoding.
…sites Replace size_t with int64_t for max_size/comp_size to match the PforWrapper API signature, and qualify pfor::PforWrapper as ::arrow::util::pfor::PforWrapper to avoid ADL ambiguity.
Aligns with Arrow buffer conventions (Buffer::data() returns uint8_t*). Removes the reinterpret_cast<char*> at the parquet encoder/decoder call sites and switches std::vector<char> compressed buffers to std::vector<uint8_t> in the unit test and benchmark. Also fixes a pre-existing size_t / int64_t* mismatch in pfor_benchmark.cc that surfaced once the buffer pointer type was tightened. Mirrors the corresponding ALP review fix on gh540-alp-pseudoDecimal-encoding.
…th validation Per Google C++ style, replace the PforVectorInfo struct with a class that has private trailing-underscore members and getter/setter accessors. Replace std::memcpy calls in Store/Load and the exception patch loop in DecodeVector with util::SafeLoadAs / util::SafeStore. Add bit_width range validation inside Load() so callers don't have to repeat the check. Updates all access sites in pfor.cc and pfor_test.cc to go through the new accessors. Caches num_exceptions() in a local in DecodeVector so the #pragma GCC unroll can still see a constant loop bound. Mirrors the corresponding ALP review fix on gh540-alp-pseudoDecimal-encoding.
Per Google C++ style, both types become classes with private trailing-underscore members and const getters, mutable getters, and setters. Updates all access sites in pfor.cc (EncodeVector, LoadView, SerializedVectorSize, SerializeVector) and pfor_test.cc to go through the new accessors. Mirrors the corresponding ALP review fix on gh540-alp-pseudoDecimal-encoding.
…, use ctor in EncodeVector - Move the num_exceptions < 0 check from DecodeVector into PforVectorInfo::Load alongside the bit_width range check, so all loaded-data invariants are enforced at the same layer. - Use PforVectorInfo's parameterized constructor in EncodeVector instead of three separate setter calls on a default-constructed instance.
Commit 00b6318 introduced ARROW_DCHECK(bit_util::IsPowerOf2(vector_size)) in PforWrapper<T>::Encode, but vector_size is int32_t and bit_util has overloads only for int64_t and uint64_t -- the call is ambiguous and the file no longer compiles. Cast to int64_t to disambiguate. CeilDiv calls in the same file already promote to int64_t implicitly via its int64_t-only signature.
Portable C++ port of FastLanes (Afroozeh & Boncz, VLDB '23) for int32_t
columnar data. No SIMD intrinsics in the kernels — the inner lane loop
is structured (contiguous loads from packed[w*kLanes + lane], contiguous
stores to transposed[r*kLanes + lane]) so the compiler auto-vectorizes
to 4-wide NEON / 8-wide AVX2 / 16-wide AVX512 without source changes.
Layout: lane-interleaved 1024-bit format per the paper. 1024 values
pack as w u32 rows of 32 u32 lanes. FL_ORDER (8x16 -> 16x8 sub-block
transpose + 3-bit-reversal sub-block reorder) is applied OUTSIDE the
kernel: FastLanesForCodec::Encode gathers input[fromTransposed32(t)]
before packing; Decode produces output in transposed order (no scatter,
output[t] == input[fromTransposed32(t)] + min within each 1024-block).
FastLanesForCodec adds Frame-of-Reference on top:
- 2048-value chunks (2 FastLanes blocks per chunk)
- Per-chunk 5-byte header: [min(4B int32 LE)] [bit_width(1B)]
- Subtract min before packing; add back on decode
- bit_width=0 path stores no payload (constant chunk)
Files:
cpp/src/arrow/util/fastlanes/fastlanes_kernels.h
- PackBlock<W>(in, out) / UnpackBlock<W>(packed, out)
- W=32 fast path: std::memcpy
- fromTransposed32 helper
cpp/src/arrow/util/fastlanes/fastlanes_for.{h,cc}
- FastLanesForCodec::{Encode,Decode}
cpp/src/arrow/util/fastlanes/fastlanes_for_test.cc
- 5 round-trip tests (narrow range, single value, full int32 range,
multiple chunks, boundary values) — all passing
CMakeLists.txt wires the test as arrow-fastlanes-for-test.
Wires the new FastLanesForCodec into the existing pfor_comparison_benchmark harness alongside PFOR, DeltaBitPack, ZSTD, LZ4, RleBitPack, and Bss codecs. New BM_FastLanesEncode / BM_FastLanesDecode functions follow the same Gen32 + ::Apply(CustomArgs) shape; REGISTER_DATASET macro picks them up for every ClickBench dataset. Notes on the comparison: - FastLanes decoder produces output in TRANSPOSED order (output[chunk*2048 + block*1024 + t] == input[chunk*2048 + block*1024 + fromTransposed32(t)] + min). PFOR/DeltaBitPack produce flat output. The benchmark measures decoder throughput head-to-head; consumers of FastLanes output must be permutation-aware (which is the FastLanes paper's intended architecture). - num_values is rounded down to a multiple of 2048 (FastLanes chunk size) inside BM_FastLanesEncode / BM_FastLanesDecode for compatibility with the existing 102400-value test sizes. Also guards add_executable(parquet-pfor-comparison-benchmark) with if(ARROW_BUILD_BENCHMARKS) so non-benchmark configurations don't fail the cmake configure step. Bench numbers on aarch64 (102400 int32, 3-run median): EventDate decode: FastLanes 20us vs PFOR 36us vs Delta 122us EventTime decode: FastLanes 24us vs PFOR 56us vs Delta 140us GoodEvent decode: FastLanes 20us vs PFOR 34us vs Delta 119us Compression ratios match or slightly beat PFOR on every dataset tested.
FastLanesForCodec::DecodeFlat unpacks into a transposed scratch buffer
per chunk and then scatters via fromTransposed32 to produce output in
original input order — output[i] == input[i] for the encoded input.
This is the FL_ORDER inverse of the gather step in Encode.
Adds:
- DecodeFlat method + round-trip test (DecodeFlatIsIdentity) covering
4 chunks of random data. All 6 round-trip tests still pass.
- BM_FastLanesDecodeFlat in pfor_comparison_benchmark, registered in
the per-dataset macro for apples-to-apples vs PFOR / DeltaBitPack
(both of which produce flat output).
Bench (102400 int32, 3-run median, aarch64):
Dataset FL Decode FL DecodeFlat PFOR Decode Delta Decode
EventDate 20 us 108 us 38 us 123 us
EventTime 23 us 113 us 57 us 140 us
GoodEvent 20 us 107 us 35 us 119 us
The transposed-kernel decode beats every other codec by 1.5-7x. The
flat-output decode pays an ~85 us scatter cost per 100K values that
makes it slower than PFOR but still faster than DeltaBitPack. The gap
is exactly the FL_ORDER scatter — the reason FastLanes' intended
architecture keeps data in transposed order through the query.
The 8x16 -> 16x8 within-sub-block transpose is mutual-inverse with the 16x8 -> 8x16 transpose, NOT self-inverse. The previous docstring on fromTransposed32 said "Self-inverse: fromTransposed32 is also toTransposed32" — that was wrong. fromTransposed32(fromTransposed32(t)) does not equal t in general; e.g. fromTransposed32(1) = 16, fromTransposed32(16) = 2. Add the actual toTransposed32 (forward-direction mapping) and fix the docstring. Callers that need to invert a gather computed with fromTransposed32 (i.e. read out[i] = transposed["the t whose fromTransposed32(t) = i"]) must use toTransposed32(i).
Adds an additive packing-mode option to PFOR. Existing vectors round-trip
unchanged (default PackingMode::BitPack); new vectors can opt in to the
FastLanes lane-interleaved bit-packing layout via the per-vector flag.
On-disk format change (backwards-compatible):
- The 1-byte bit_width field of PforVectorInfo now packs two values:
bits 0..5 = the actual bit width (range 0..32 fits in 6 bits)
bit 7 = packing-mode flag (0 = BitPack, 1 = FastLanes)
bit 6 = reserved
- Legacy encoders only wrote the bit width, leaving high bits clear,
so they decode as PackingMode::BitPack via the new Load.
- PFOR header (page-level) is unchanged.
API:
- New enum class arrow::util::pfor::PackingMode { BitPack, FastLanes }.
- PforVectorInfo gains a packing_mode field and getter/setter.
- PforCompression<T>::EncodeVector takes an optional PackingMode (default
BitPack). FastLanes mode is only honored when num_elements equals the
FastLanes block size (1024) and T is 32-bit; otherwise it falls back
to BitPack per-vector (so tails and 64-bit values continue to work).
- PforCompression<T>::DecodeVector reads the per-vector flag and
dispatches between arrow::internal::unpack and the FastLanes kernel.
- PforWrapper<T>::Encode takes an optional PackingMode threaded down to
EncodeVector.
Decode-side perf (fused gather + FOR-add + SafeCopy):
The FL_ORDER inverse needs toTransposed32(i) — note: NOT
fromTransposed32(i), the two are mutual inverses, not self-inverse.
The scalar gather over can't be SIMD-vectorized, so
PFOR+FastLanes decode is ~2-3x slower than PFOR+BitPack end-to-end
despite the kernel itself being competitive. The win is only available
when the downstream consumer can work with data in FastLanes transposed
order (i.e. relax the flat-output contract).
Tests: 5 new tests in PforPackingModeTest cover round-trip identity for
both modes, the partial-tail fallback to BitPack, mixed-mode round-trip
through PforWrapper, and the bit_width=0 (constant vector) path. All 30
PFOR tests pass.
Benchmark: BM_PforFastLanesEncode / BM_PforFastLanesDecode added to
pfor_comparison_benchmark.cc, registered per dataset alongside the
existing 8 codec variants.
For FastLanes-encoded vectors the decoder previously always paid a
1024-element scalar FL_ORDER gather to produce flat output. That gather
is what made pfor+fastlanes 2-3x slower than pfor+bitpack overall, even
though the FastLanes unpack kernel itself is competitive.
The FastLanes paper's intended decode path is to NOT do that scatter at
all: keep the data in FastLanes stream order and let downstream
operators be permutation-aware (apply fromTransposed32 lazily, when
they need original index). This commit exposes that path.
API:
- New enum class arrow::util::pfor::OutputOrder { Flat, Transposed }.
- PforCompression<T>::DecodeVector and PforWrapper<T>::Decode take an
optional OutputOrder (default Flat, backwards-compatible).
- OutputOrder::Transposed only affects FastLanes-encoded vectors.
BitPack vectors have no permutation to skip, so they always produce
flat output regardless of the argument (mixed pages with a BitPack
tail end up flat in the tail, transposed in the full blocks).
Decoder paths in DecodeVector when packing_mode == FastLanes:
- Flat (existing): unpack -> scratch transposed[] -> fused
values[i] = SafeCopy(transposed[toTransposed32(i)] + FOR)
The toTransposed32 gather is scalar, breaks auto-vec.
- Transposed (new): unpack -> scratch transposed[] -> sequential
values[t] = SafeCopy(transposed[t] + FOR)
Pure sequential read/write, auto-vectorizes cleanly. Exceptions are
patched at toTransposed32(pos) so the stored-flat positions land in
the right transposed slots.
Tests: 4 new tests in PforOutputOrderTest cover (a) transposed output
satisfies the FL_ORDER relation, (b) manual inversion of the
permutation reconstructs the input, (c) BitPack vectors ignore the
Transposed request, (d) wrapper-level transposed decode across many
vectors. All 34 PFOR tests pass.
Benchmark: BM_PforFastLanesDecodeTransposed added, registered per
dataset. On 18 ClickBench-style datasets (102400 int32 each):
pfor+bitpack 33-57 us
pfor+fastlanes (flat) 98-108 us (0.34-0.53x — slower)
pfor+fastlanes (transp) 20-23 us (1.6-2.5x faster than bitpack)
The transposed path beats every other codec measured in the comparison
benchmark on every dataset.
Both lane assignments break the format's single 1024-long dependency chain into 32 independent ones, and both therefore decode with one vector add per row. They differ in what a lane's predecessor is, and that decides the size: striding 32 positions apart stores differences the format never would, which costs 2.1-3.4x the bytes on the three correlated columns of the benchmark corpus. Holding a contiguous run of 32 values per lane stores exactly the format's differences, and lands within 1.5% of DELTA_BINARY_PACKED's size across the corpus. One encoding cannot mean both, so the encoding writes the second and the stride stays a kernel-level comparison. Measured against the DELTA_BINARY_PACKED decoder in the same binary, over 33 columns at -O2: decode 2.9x faster on 33 of 33, encode 2.6x on 33 of 33. The page framing costs under 2% of decode and no bytes. The corruption test moves with the layout: block widths now open the payload rather than sitting 128 bytes in, and the entry-point width lives inside a block, so there are two positions to corrupt instead of one. Two tests cover what the arrangement adds -- a page whose last block carries no payload, so the entry points end it, and entry points far enough apart to need the full width.
Compares the portable sequential bit-unpacker (new, arm 1), the in-tree interleaved kernel (arm 2), and Arrow's dispatched unpacker (arm 3, called through its real header against the built library) on instructions/value and IPC per bit width, via self-process perf_event_open counters with correctness gates on every arm. Arm 1 and arm 3 are fixed, prebuilt inputs; only arm 2 recompiles between the driver's -O2 and -O3 builds. Not wired into CMake: perf_event_open is Linux-only and needs counter access the build system can't guarantee everywhere. Build commands are documented in driver.cc's header comment.
InterleavedBitPackingLayout and InterleavedRequestIgnoredForInt64 built WriterProperties directly and never called enable_pfor_encoding(), so both threw the preview-feature guard instead of exercising the layout.
|
Thanks for opening a pull request! This pull request has been automatically converted to a draft because its title doesn't match Arrow's required format. If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or After updating the title, you can mark the pull request as ready for review. See also: |
github-actions
Bot
added
Component: Parquet
Component: C++
awaiting review
prtkgaur
changed the title
Sweeps ARROW_USER_SIMD_LEVEL across SSE4_2/AVX2/AVX512 on a single native build of parquet-pfor-comparison-benchmark, with identical repetitions at every width, and combines the three runs into one tarball. Meant to be handed to someone with x86 hardware so the sequential-vs-transposed decode ratio can be measured at each register width from one benchmark invocation.
Arrow's shipped PFOR decoder, plain PFOR through the FastLanes interleaved container in file order, and the same container with the paper's lane assignment now all run against the existing TPC-DS, ClickBench, TPC-H, NYC taxi, and synthetic-shape column corpus in one benchmark binary, so the layout question is measured on data instead of on a synthetic per-bit-width sweep. The lane-assignment arm exists to price the gather it forces, not to recommend it -- plain PFOR has no dependency chain for that ordering to help.
The decode arm this benchmark used as the sequential baseline runs the PFOR encoder with its default options, which let the planner difference a vector whenever its cost model prefers that. On a sorted or correlated column it does, and the resulting decode also walks a serial prefix sum. Ratios taken against a layout arm that never deltas were therefore pricing two decisions at once: on the delta-shaped columns the shipped decoder spends 3.3x-4.3x of its own plain-mode time on the prefix sum alone, which is large enough to dominate anything the layout contributes. The encoder already exposes both knobs the comparison needs -- a flag to decline delta and an advisory packing mode, one of whose values is the lane-interleaved container. So add two arms that hold delta off and vary only the layout, both going through the production encoder and decoder rather than a benchmark-local reimplementation. Their payloads come out byte-identical on all 43 int32 columns, which is the check that the layout is the only difference between them. The sweep script picks up both arms and its header now says which pair to quote for the layout question.
bpacking_simd_avx512.cc is the one bit-unpacking translation unit never
migrated off the legacy generated kernels in
bpacking_simd512_generated_internal.h. Those build their SIMD input register
from an initializer list of individual scalar loads and carry no
ARROW_FORCE_INLINE, so each step is an out-of-line call. The width-12 uint32
body is 128 instructions per 32 values, of which six are actual unpack math
(4x vpsrlvd, 2x vpandd); 32 are element-by-element register assembly via
vmovd/vpinsrd and 55 are scalar bit-splicing in GPRs. It references xmm 44
times and zmm five times.
Measured on Granite Rapids over 102400 values via exported
unpack_bias<uint32_t>, forcing the target with ARROW_USER_SIMD_LEVEL so only
the kernel source varies (geomean widths 1..31, GiB/s):
scalar 9.74
sse4_2 41.38
avx2 40.97
avx512 6.54 0.16x of avx2, 0.67x of scalar
Being slower than the scalar kernel is the sanity check that says bug rather
than tradeoff. Width 32 is the control the bug cannot reach (memcpy path): all
four targets agree at 46.5-46.8.
Because ARROW_RUNTIME_SIMD_LEVEL defaults to MAX, every AVX-512 machine
preferred this kernel. aarch64 has no AVX-512 and was never affected, which
accounts for PFOR decoding at ~30 GB/s on ARM against ~6 GiB/s on x86.
With the cap, default dispatch goes from 6.60 to 50.74 GiB/s at width 12 and
is bit-identical to the scalar kernel across all 32 widths and both entry
points.
This is a shared-library dispatch policy change and wants an upstream
decision, so it is kept as its own commit. Re-enable once the 512-bit kernels
issue real vector loads. Recorded in the comment so nobody repeats it: naively
pointing the AVX-512 TU at the Kernel<> machinery used by
bpacking_simd_{128,256}.cc compiles and is bit-exact but measures 1.17 GiB/s,
5.6x worse again, because most widths land on is_oversized() -> NoOpKernel and
fall through to the naive path.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
FL_ORDER places a contiguous run of 32 values in each SIMD lane so a delta
chain splits into 32 independent 32-long chains. Parquet cannot hand that
order to a reader -- definition/repetition level association, cross-column row
alignment, row-range predicate pushdown and sub-range reads all depend on
value order, and the first of those breaks silently -- so file order has to be
restored on every decode. This makes that restoration cheap.
The naive implementation unpacks into a 4 KiB stack grid and runs a separate
Transpose32x32 over it, materializing the grid purely to read it straight
back: 128 extra 32-byte stores and 128 extra loads per block on top of the 128
stores the output needs. Folding the permutation into the kernel's store
addressing instead is a trap and is documented as such -- out[lane * 32 + row]
puts the 32 lanes of a row 128 bytes apart, turning each contiguous 32-byte
store into eight 4-byte scatters, 1024 stores per block instead of 128.
What works is doing the transpose in registers on values that never reach
memory in grid form. UnpackBlockFlToFileOrder fills the same 8x8 register
block Transpose32x32Avx2 already used, directly from the unpack, and runs the
same unpacklo/unpackhi/permute2x128 ladder. Per-block load and store counts
are then identical to the file-order kernel's; the only thing FL_ORDER pays
over file order is the shuffle ladder. lb is the outer loop deliberately: a
fixed lb writes one contiguous 1 KiB run, whereas rb outer would stride 128
bytes across the whole block.
The same fusion is applied to the delta path, where unpack, prefix sum and
permute now happen in one pass. Measured over 43 real columns at 400 KiB
(GiB/s), the order-restoration ladder:
kSeparate unpack -> grid -> prefix sum -> transpose out 21.6
kFused prefix sum and permute in one pass, reads grid 21.57
kFusedUnpack no grid at all 31.30
Notably the order-agnostic arm that skips repair entirely but still writes and
re-reads a grid measures 16.05 -- slower than doing the full permutation
without one. The memory round trip costs more than the permutation.
Against Arrow's DELTA_BINARY_PACKED at 3.77 GiB/s that is 8.31x. Holding the
layout fixed and measuring what delta costs over frame-only on the same
columns: sequential 11.24x (one 1024-long chain), interleaved file order
2.33x, FL_ORDER 1.55x.
For plain PFOR, where there is no chain to break, FL_ORDER has nothing on the
benefit side and still owes the transpose; the transpose costs 43% at L1, 10%
at L2 and 3% at DRAM. kFlOrderRaw is kept as a harness self-check: it runs the
same PackBlock/UnpackBlock as kFileOrder against a grid merely filled
differently at encode time, so the two must decode at equal speed, and they
agree within 0.2%.
pfor_comparison_benchmark.cc gains the arms these numbers come from.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The scripts that produced the numbers in the two preceding commits, so the
measurements are reproducible rather than asserted.
run_width_matrix.sh, run_width_matrix_v2.sh, time_width_matrix_v2.sh
sweep bit widths 1..32 across register-width targets (-march=nehalem /
haswell / skylake-avx512). This is the matrix that showed two of three
width tiers selecting a kernel worse than scalar: at SSE4.2 the "SIMD"
unpacker measures 3.31 GiB/s against its own scalar kernel's 13.18.
run_zmm_points.sh
forces ARROW_USER_SIMD_LEVEL per target on one binary, which is how the
AVX-512 dispatch numbers in "Cap the bit-unpack SIMD dispatch at 256
bits" were isolated to kernel source rather than to the call site.
build_transpose_ab.sh, run_transpose_ab.sh, ab_compare.sh
A/B the order-restoration ladder (kSeparate / kFused / kFusedUnpack)
within one binary, arms alternating inside each repetition so cache
warmth and output address cannot separate them.
wait_quiet.sh
blocks until the box is quiet enough to time on; run_transpose_ab.sh
depends on it. Checks three conditions rather than loadavg alone,
because loadavg is a lagging 1-minute average and a benchmark that has
just started shows a low load while already owning a core.
Build directories are deliberately left untracked.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The ARM handoff instructions referenced build.sh, fl5_corpus and seq_granularity, none of which were ever committed -- they lived outside any git repository, so the instructions could not be executed. This adds the sources. build.sh now locates the checkout from its own path, so only ARROW_BUILD has to be set, and it preflights the three headers it needs instead of failing with a wall of compiler errors. seq_granularity is the calibration probe, not an optional extra: calling Arrow's exported unpack_bias once per 1024-value block rather than once per buffer costs 1.27x at L1 and L2 on x86, a handicap the seq_simd arm pays and the header-inlined interleaved arms do not. Without dividing it out, intlv/seq_simd reads 1.43x at L2 where the layout-only effect is ~1.13x. The divisor is toolchain-specific and has to be measured locally. The README also records a correction to the working-set ladder. The point labelled DRAM is not DRAM: the reference machine is a Xeon 6975P-C with 2 MiB of L2 per core and 480 MiB of shared L3, so a 32 MiB working set is L3-resident and that row measures L3 bandwidth. Additional points, and a concurrency sweep, are still to come. Binaries are gitignored; x86 reference outputs are checked in so the aarch64 run has something to compare against. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The AVX-512 bit-unpack kernels build their SIMD input register from a list of scalar loads and measure 0.67x of the scalar kernel, so bit-unpack dispatch is now capped at 256 bits. A sweep leg that asks for the 512-bit level therefore resolves back to the AVX2 kernel and prints a column identical to the 256-bit one, which is worse than not running it: the run would read as three independent measurements and support a claim about 512-bit registers that no kernel in the binary made. Ask for 128 and 256 only, and say in the header what has to change before the leg comes back.
The register-width sweep could not answer the layout question it was written for. Two defects, both in what the runner was handed. The footprint ladder reached only BM_PforPlainSeqDecode and BM_PforPlainInterleavedDecode; the three grid arms, including the candidate, stayed pinned at the single 102400 point. The grid's advantage is a compute effect, so it converts to time only while stores are not the limit: 1.76x at 16 KiB, gone by 1.5 MiB. One mid-size point shows neither the size of the win nor the size of the permutation tax that cancels it. Apply LayoutArgs to all four comparison arms. The sweep filter also omitted BM_InterleavedPforFlOrderRawDecode, which is the control that makes the result readable. A flat candidate ratio has two explanations -- the grid's unpacking is no cheaper, or it is much cheaper and the permutation spends the whole win -- and they are indistinguishable without it. They point at different fixes. Measured at 16 KiB the grid wins 1.75x and the permutation charges 1.77x; the first run could see neither number. Add pfor_layout_tables.py to turn a returned tarball into the four-arm tables directly, since the arms are only meaningful read against each other. It fails loudly when an arm or the ladder is missing rather than printing nan, and it checks fl_unpk against intlv -- same kernel, same wire bytes, only the grid fill differs, so a gap there bounds what every other ratio can mean. Add PFOR_LAYOUT_RERUN.md with the build and verification steps. The critical one is -DARROW_SIMD_LEVEL=AVX2: the fused FL_ORDER transpose is gated on __AVX2__ through ARROW_TRANSPOSED_DELTA_AVX2, so a default SSE4_2 build has no fused kernel at all and the candidate arm silently measures a scratch-materialising fallback instead. That fallback costs 2.35x against the fused kernel's 1.08x, which is where the first run's 0.55x came from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…wall
The corpus harness measured at three points, the widest of which was
DRAM. That is the wrong ladder for this question: the grid layout's
advantage is a compute effect, so it survives only while stores are not
the limit, and the interesting transition happens inside the cache
hierarchy rather than at the memory boundary. Replace {L1, L2, DRAM}
with {16 KiB, 400 KiB, 1.5 MiB, 4 MiB, 32 MiB} and drop the DRAM point
entirely -- once bandwidth is the limit every layout converges and the
point carries no information about the layout.
Add a store-only sixth arm. Reading the other five against it is what
turns "the grid executes fewer instructions" into a claim about time:
an instruction saving converts only below the store wall, and without
the wall measured on the same machine there is nothing to check that
against.
Set max_read_bytes on the sequential arm. Arrow's real callers always
set it, and it is what lets the SIMD kernel run to the end of the block
instead of bailing one iteration early -- the kernels overread by up to
a register -- and handing the tail to the scalar epilog. Leaving it at
-1 handicapped the baseline against the real decoder. This makes the
sequential arm faster and therefore works against the grid layout, not
for it, which is the direction an error here should run.
Also record two caveats in interleaved_pfor.h that were being carried in
conversation rather than in the tree: the corpus columns are synthetic
generators rather than captured data, and the grid unpack kernel has no
runtime SIMD dispatch, so its register width is frozen by
ARROW_SIMD_LEVEL at compile time while the sequential path it is
compared against dispatches to AVX2 at runtime. Comment-only.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four drivers each carried their own copy of the column list and the arm regex, and all four named the same wrong pair: the production PFOR decoder, which patches exceptions, against two arms from arrow/util/fastlanes/interleaved_pfor.h, which has no exception handling anywhere in it. The interleaved side of that quotient is doing less work, so the number is not a layout result. The correct pair -- the same production encoder and decoder differing only in PackingMode -- had already landed in the same benchmark file when those drivers were written, and its own header said so. The arm sets now live in bench_arms.sh, which states once which groups may be divided by which and why: the layout pair through production, the ordering trio on the standalone path where the file-order arm is the only valid baseline, the destination-policy pair, and the shipping default. The header comment in interleaved_pfor.h that recommended the invalid comparison is replaced by the reason it is invalid, and the benchmark file says the same above the three arms it applies to. Three drivers go away rather than being fixed. One was superseded by a corrected rebuild of itself, whose header documents the original's results as a binary compared against itself. One existed only to add -mprefer-vector-width=512 back to a sweep that had forgotten it; the replacement sets it at every point. One timed a single process per point minutes apart, which its own header called superseded by the alternating protocol. The surviving build driver also drops its 512-bit legs, for the reason the register-width sweep already dropped its own: bit-unpack dispatch is capped at 256 bits, so a 512-bit point hands the sequential arm the 256-bit kernel back while the interleaved kernel really does widen.
…ling The harness had grown in two directions at once. One version replaced the destination-size ladder (L1..L3big) with a page-scale one: the decode call is held at a default page while the source and the destination are rotated over separate arenas, so a "scan" point streams distinct cold packed bytes instead of re-reading one hot copy. That is what closed the buffer-placement artifact the old ladder was measuring. The other version added a sixth arm, pure_st, which writes the same bytes to the same place with no unpacking at all and so prices the store wall the decode arms cannot get under. Neither subsumes the other, so both are here. pure_st writes into the same rotating destination slice the decode arms use, and is exempt from the bit-exact check because it is a ceiling and not a decoder -- it also reads nothing from the packed source, which is why the scan points read it as an upper bound rather than as a comparand.
The harness condemned an entire run when any single column's timing control missed 1.00x by 5%. That is too strict to survive a boosting clock over a four-minute phase: our own reference ladder trips it at four of six working sets while its control geomeans stay inside 1.008x, and an outside run of the same binary was reported as unusable for the same reason. A point is now quotable when its control geomean is within 1.5%, at least four columns in five tie within 5%, and recomputing the headline ratio over only the tying columns moves it less than 2%. Every input to that verdict is printed, so a reader can apply their own thresholds, and the failing criterion is named per point. A nonzero exit now means no point in the run was usable rather than that one working set was noisy. Applied to the runs we have, this rejects only the largest-destination working set, which no result we quote depends on. build.sh also no longer fails when a harness source is absent from the tree; it skips it with a note and errors only if nothing was built.
The interleaved pack and unpack kernels hold no intrinsics: they are portable C++ whose register width comes from the flags of the translation unit that compiled them, and they had no runtime dispatch at all. That left ARROW_SIMD_LEVEL deciding their width, which on a default x86 build is SSE4_2, so the interleaved kernel ran in XMM registers while the sequential unpacker it gets compared against dispatched to AVX2. Choosing an instruction set for a kernel like this is choosing a translation unit, so the kernel source is now compiled once per level: a baseline leg, plus one vector source registered for both SVE128 and AVX2 the way bpacking_simd_256.cc already is. Each leg builds its own 33-entry width table, so an entry points at a body carrying that leg's flags, and pfor.cc picks between the legs with DynamicDispatch exactly as bpacking.cc does. The kernels take an unused Arch type parameter to make that safe. Without it, one width instantiation compiled at two instruction sets shares a mangled name, the linker keeps one definition of the pair, and every leg silently becomes the same code with no diagnostic. Both leg sources are compiled at -O3, which no other source in Arrow's CMake asks for. These kernels are unusually level-sensitive -- at width 16 gcc 11.5 emits no vector operations at -O2, 81 at Release's -O2 -ftree-vectorize, and 513 at -O3 -- and -O3 is the level every published figure for this layout was measured at.
The harness took an optional dataset filter and an optional CSV path, so two people running it could produce outputs that are not comparable, and the name said nothing about what it measures. Rename it to layout_benchmark, drop argv entirely, and write the CSV to a fixed filename beside the text output. build.sh now builds this one binary; the README describes what it prints instead of the superseded three-point ladder.
Comment and naming pass over the interleaved/FastLanes layout work, plus one factual correction. No decoder or kernel behaviour changes: every C++ and shell file except the four listed below is byte-identical once comments are stripped. Comments. Replaced the boxed "// ====" banners with Arrow's own section separator (a "// " line of 70 dashes), which is what the rest of the tree uses; the boxed form appears in only one vendored subtree upstream. Removed the ALL-CAPS shouting from prose and from printed banners, and dropped a diary section from the collaborator hand-off script that recorded how an earlier revision of that script was wrong -- the technical content it carried (the sequential decoder's runtime dispatch versus the grid kernel having no dispatch dimension, the objdump register census, the AVX512 cap in bpacking.cc) is kept in sentence case. Rewrote the width-0 early-return comment in the PFOR frame search so it says what the case is rather than editorialising about it. Naming. Dropped "arm" as a word for a benchmark variant throughout, in identifiers and in output text: kArms/kArm -> kVariants/kName in the layout harness, ARMS -> BENCHES/DECODERS in the two table generators, ARM_SETS/ARMS_* -> GROUPS/GROUP_* in the comparison scripts, and bench_arms.sh -> bench_groups.sh. Also dropped "verdict" for what is just a ratio or a pass/fail column. Correction. The layout harness header and its README claimed the fl_unpk/intlv timing control "has to measure 1.00x" and that the binary exits non-zero otherwise. It does neither. The control feeds a three-part validity gate per working-set size -- tie in aggregate, four columns in five tying individually, and bounded drift when the non-tying columns are dropped -- and the exit code is non-zero only when no point passes at all. Both places now state that. Files whose code changed, all of it the renames above: ab_compare.sh, pfor_layout_tables.py, gen_tables.py, layout_benchmark.cpp. pfor_corpus_internal.h is the column corpus lifted out of pfor_comparison_benchmark.cc so the standalone harness can include it instead of carrying a second copy. Verified: all nine translation units syntax-check (both ISA legs of the per-instruction-set kernel source), layout_benchmark builds and links against libarrow and runs, clang-format is no worse than HEAD on every file under cpp/, and bash -n / ast.parse pass on every script.
The previous commit replaced the boxed "// ====" separators with Arrow's
"// " + 70-dash form, but two files on this branch kept theirs: the column
corpus header, which was still untracked when that scan built its file
list, and the encoding integration test, which the scan's path patterns
did not match.
Found by taking the census that decides the convention in the first place
and subtracting the files known to be upstream:
grep -rl '^// ={10,}' --include=*.cc --include=*.h cpp/src
Arrow now has 7 such files: four in the vendored ODBC subtree, one in
util/, and these two. That leaves only upstream files.
Comment-only. Both files are byte-identical once comments are stripped,
clang-format is unchanged at 0 warnings each, and the corpus header still
compiles standalone.
The width-matrix script's header argued with a predecessor that no longer exists: twenty-five lines of sha256 digests establishing that a deleted version's "-O2" points were really -O3. What a reader needs from that is the constraint it produced, so the header now states it directly -- the level is set explicitly at every point, because a local flags override means Release does not mean -O2 here, and because the cache variable would otherwise carry one point's flags into the next. The rest is capitalised emphasis in prose, all of it readable in lower case: five spellings of "ARE the signed values", one "SYNTHETIC", a shouted section heading, objdump's "OWN" labels, and "ONE batch". The bias comment in the bit-packing kernel said "The whole point:" before saying the point. Comments only. Stripping comments leaves all five files byte-identical to their previous revision, and clang-format reports no new warnings.
Comment, string-literal and prose changes only. Every source file is byte-identical once comments are stripped, except two whose sole change is inside a string literal: one printf header and one error message. The harness still compiles at -std=c++20 and both scripts pass bash -n. Comments that described what the harness used to do now state what it has to do. The arena preamble kept every measurement -- inputs and outputs both carved at 4096-byte-aligned offsets, the allocator placing blocks differently as the request grows, and the 1.36x-to-0.47x swing between bit widths 11 and 12 at the largest point -- but frames them as the reason the address has to be pinned rather than as an account of an earlier version. The ladder comment keeps the reason both footprints are named on every line. In the low-outlier test, "the old frame" now names what it meant: a frame pinned to the minimum, which is the classic PFOR choice the test contrasts against, not a previous revision of this code. The sweep script's error message points at the requirement, one binary per level, rather than at the invocation form it replaced. Two corrections: - The validity header printed "must be 1.00x" directly above the three tolerances that are actually enforced: 1.5% on the control geomean, 5% per column with four in five required to tie, and 2% of drift. It now says the control has to tie and defers to the legend for the tolerances. - The README offered the checked-in x86 recording "for comparison" without saying it came from an earlier version of the harness. That recording labels three points L1/L2/DRAM by output size alone and has no store-only reference, so its rows do not correspond to the six points this harness prints, and lining them up is the exact mislabelling the ladder comment warns about. The README now says what the file is good for and what it cannot be read as. The width-verification comment claimed a width that did not materialize is always reported, above a check that covers only the 256-bit points. The check is right -- the 128-bit points build at SSE4_2 where __AVX2__ is undefined and ymm cannot be emitted -- so the comment now says why only one width can miss. Also two remaining arrow glyphs to ASCII in the rerun notes.
The sweep compares three unpack kernels at one bit width, and it only means anything if all three are reached the same way. Two of them already were: arm 1 goes through kSeqUnpack's function-pointer table, arm 3 crosses into libarrow.so. The interleaved arm was called inline from the timing lambda instead. That is not a neutral difference. Inlined, sixteen fully-unrolled block bodies land in one function and gcc stops vectorizing them, so the arm reported 6.6-7.5 instructions per value at every optimization level and looked insensitive to flags. It is not the kernel that is insensitive -- it is a kernel the vectorizer gave up on. Behind a function boundary the same source reports 2.14 instructions per value at -O2 -ftree-vectorize and 1.07 at -O3, and the level sensitivity appears where it belongs. The asymmetry only ever hurt this arm, because it is the only one of the three whose speed depends on autovectorization: the sequential kernel is a per-width generated body and the shipped kernel is already compiled.
UnpackBlockFlToFileOrder existed only under AVX2, so every Arm target took the portable fallback: unpack a 4 KiB grid to memory, then transpose out of it. Every value stored twice and the block traversed twice, for the one arm whose whole job is the permutation. This adds the NEON counterpart. FlUnpackRowSliceNeon is the four-lane twin of the eight-lane x86 slice, and the tile is four rows by four lanes rather than eight by eight because that is the register width -- four times as many tiles per block, but the same number of loads and stores, since a 16-byte store carries four output values either way. On the 43-column corpus the arm gains 1.25x at the project's release flags and 1.13x at -O3, and 1.43-1.47x at the working set that writes 48 MiB once and never reads it back, which is where the grid's second traversal cost the most. It is also now insensitive to optimization level, at 19.8 GiB/s either way, because intrinsics do not depend on the vectorizer. Two of 258 points regress at the release flags and 33 at -O3, all at widths where the fallback's unpack was already close to a copy and the fused tile pays the shuffle ladder for nothing. The arm still reads about 0.65x of the file-order decoder, up from 0.58x. That remainder is the permutation itself: 1.48 instructions per value against file order's 0.94. Pairing the stores into 32-byte vst1q_u32_x2 was tried and is 0.63-0.80x of the four-row tile -- it lowers to the multi-register st1 form, which has lower throughput here than plain stores. Checked against UnpackBlock followed by Transpose32x32 at all 32 widths with and without a bias, and every corpus column still compares equal to its original values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how to contribute here:
Please remove this line and the above text before creating your pull request.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?
This PR includes breaking changes to public APIs. (If there are any breaking changes to public APIs, please explain which changes are breaking. If not, you can remove this.)
This PR contains a "Critical Fix". (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)