joechenrh opened a new pull request, #937:
URL: https://github.com/apache/arrow-go/pull/937
## Summary
The streaming value buffer's `Fill` read only `need` bytes per call. The
PLAIN
byte-array / FLBA decoders call `Fill` per value (a 4-byte length, then the
value),
so the value reader gets ~2 `Read` calls per value — **~5.1M `Read` calls to
decode a
32 MiB page of ~9-byte values**. When the reader is a block decompressor
(zstd/gzip),
each call just copies a few bytes out of an already-decoded block, so decode
is
dominated by call overhead and runs **1.4–1.8× slower** than the
materialized path for
small values.
`Fill` now **reads ahead** — filling toward the end of the current chunk
(bounded by the
value region) instead of stopping at `need` — so the reader is called
**~once per chunk
(~260 calls for that page instead of ~5.1M)**. To keep peak memory at one
chunk despite
reading ahead, `Recycle` **compacts** the unconsumed tail to the front of
the chunk once
the consumed prefix passes half the chunk, instead of allocating a fresh
chunk on rotate.
**Peak is unchanged (~1 MiB, the `DefaultBufferSize` window).**
## Correction
I need to correct — and apologize for — something I wrote while landing the
streaming
decode in #880:
> I prototyped a read-ahead buffer to close the zstd gap and measured ≈0
improvement
> (the cost is the extra copy, not the read count), so I dropped it.
That conclusion was wrong, and worse, it undid something that had already
been working.
An earlier iteration of this value buffer *did* read ahead, correctly; it
was dropped when
the buffer was later redesigned to bound peak memory to one chunk. The
re-prototype behind
the "≈0 improvement" claim was then **broken** — read-ahead that let
`Recycle` rewind the
cursor and drop the unconsumed tail, which misaligns the value stream — so
its "no gain"
was an artifact of the bug, not evidence that read-ahead is ineffective. The
read *count*
is in fact a large part of the small-value cost, and removing read-ahead
shipped a
**1.4–1.8× small-value decode regression** for block-compressed pages:
hidden behind the
fetch in IO-bound pipelines, but a real cost for decode-bound readers (local
files,
CPU-bound consumers). Sorry for removing a working optimization and then
drawing the wrong
conclusion from a broken re-prototype. This PR restores read-ahead, adapted
to the chunk
model with the tail-carry the re-prototype lacked, and adds a test for that
path.
## Results
Decode-only, warm; single ByteArray column, zstd, 32 MiB pages:
| value size | before (Read per value) | with read-ahead |
| ---------- | ----------------------- | --------------- |
| 9 B | ~1.7× | **~1.2×** |
| 33 B | ~1.5× | **~1.2×** |
| 129 B | ~1.4× | **~1.25×** |
- `Read` calls (9 B page): **~5.1M → ~260**.
- Peak allocator use: **unchanged (~1 MiB)** —
`TestPageStreamingPeakMemoryBounded` still
passes at ~1 MiB for a 4 MiB page.
- Correctness: the existing streaming↔materialized round-trip tests (all
codecs, V1/V2,
required + nullable) still pass; adds `TestStreamBufferReadsAhead` and
`TestStreamBufferReadAheadSurvivesRecycle` (the tail-across-`Recycle` path
the earlier
prototype got wrong).
The win is largest on tiny values (most `Read` calls); larger values were
already near
the floor. The remaining **~1.2× is expected**: it is the per-value
`Fill`/`Advance` calls
through the `ValueBuffer` interface versus the materialized path's inline
slicing — the
structural per-value overhead already noted in #880 (tiny values +20–45%).
Read-ahead
removes the part that the read count *added*; it does not remove the
per-value
abstraction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
