perf(parquet): decode BSS FLBA into contiguous storage - #1172
perf(parquet): decode BSS FLBA into contiguous storage#1172fallintoplace wants to merge 3 commits into
Conversation
|
Since this is a draft, i'll hold off on further review until it's marked ready |
zeroshade
left a comment
There was a problem hiding this comment.
The contiguous cold-output allocation is correct and substantially reduces allocations when every output entry is empty. Ownership across decoder resets, partial decoding, and spaced decoding also looks sound.
However, the current loop regresses the existing all-reusable steady-state path by about 10–11% on arm64 and can overallocate dramatically for mixed reusable output. I included measurements and a suggested fast-path/slow-path structure inline.
Targeted encoding and race suites pass. File and pqarrow integration tests were blocked only by the unavailable PARQUET_TEST_DATA checkout. GitHub currently reports no CI checks for this head.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Arrow Go maintainer. After you've
addressed the point above and pushed an update, an Apache Arrow Go
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Arrow Go handles maintainer review:
CONTRIBUTING.md.
| for idx := range output { | ||
| if cap(output[idx]) < dec.typeLen { | ||
| if storage == nil { | ||
| storage = make([]byte, (len(output)-idx)*dec.typeLen) |
There was a problem hiding this comment.
This sizes the backing block from all remaining outputs rather than from the outputs that actually need storage. In a 65,536-value, width-16 batch where only out[0] lacks capacity, the merge base allocates 16 B while this code allocates 1,048,576 B.
The additional storage state in this loop also regresses the existing all-reusable steady-state benchmark on arm64:
- 1,024 values: approximately 1,060 → 1,174 ns/op
- 65,536 values: approximately 69.0 → 76.2 µs/op
Both results were stable across six 500 ms runs with GOMAXPROCS=1.
Since preserving reusable caller storage is part of this optimization, could the existing fast loop remain unchanged until the first insufficient-capacity entry, then delegate the suffix to a cold helper that counts missing entries and allocates exactly missing * typeLen bytes? Please also add a mixed-reuse benchmark or allocation assertion so this case remains covered.
Rationale for this change
BYTE_STREAM_SPLITdecoding forFIXED_LEN_BYTE_ARRAYcurrently allocates a separate byte slice for every output value when the output has no reusable capacity. A 65,536-value batch therefore performs 65,536 decoder allocations.What changes are included in this PR?
Apple M1 Pro, GOMAXPROCS=1, 65,536 values:
Bytes allocated stay unchanged. The existing reusable-output benchmark also remains allocation-free and improved by about 8%.
Are these changes tested?
PARQUET_TEST_DATA=$PWD/parquet-testing/data go test ./parquet/...go test -race ./parquet/internal/encodinggo vet -composites=false ./parquet/internal/encodingAre there any user-facing changes?
No.