zeroshade commented on code in PR #1172:
URL: https://github.com/apache/arrow-go/pull/1172#discussion_r3799196122
##########
parquet/internal/encoding/fixed_len_byte_array_decoder.go:
##########
@@ -173,11 +173,18 @@ func (dec *ByteStreamSplitFixedLenByteArrayDecoder)
Decode(out []parquet.FixedLe
return 0, errors.New("parquet: eof exception")
}
- for i := range out {
- if cap(out[i]) < dec.typeLen {
- out[i] = make(parquet.FixedLenByteArray, dec.typeLen)
+ output := out[:toRead]
+ // Lazily allocate one backing buffer while continuing to reuse
caller-provided storage.
+ var storage []byte
+ for idx := range output {
+ if cap(output[idx]) < dec.typeLen {
+ if storage == nil {
+ storage = make([]byte,
(len(output)-idx)*dec.typeLen)
Review 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.
--
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]
