flate: use Peek instead of ReadByte for the bufio.Reader decode path - #1169
Conversation
huffmanBufioReader filled the bit buffer with fr.ReadByte() once per input byte. bufio.Reader.ReadByte does not inline, so each call is a barrier that spills the fill loop's fb/fnb registers -- roughly 1.85x slower than the *bytes.Reader variant, whose ReadByte inlines to a slice index. Read from a Peek window instead: peek bufio's buffered slice and index it directly (pbuf[pos]), refilling when drained. Peek only views the buffer, so consumed bytes are Discard()ed before every return, keeping the reader position correct for nextBlock/moreBits and the gzip trailer. peekBufio refills from at most one underlying Read (never Peek(Size()), which would block a streaming/sync writer until the whole buffer fills). Only the *bufio.Reader variant changes; the other four generated variants are byte-identical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe flate bufio Huffman decoder now consumes buffered input through ChangesBufio Inflate Decoding
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HuffmanDecoder
participant BufioReader
participant CompressedInput
HuffmanDecoder->>BufioReader: Peek buffered bytes
BufioReader->>CompressedInput: Refill when needed
HuffmanDecoder->>BufioReader: Discard consumed bytes
HuffmanDecoder->>HuffmanDecoder: Decode Huffman symbols
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Peek instead of ReadByte for the bufio.Reader decode path
klauspost
left a comment
There was a problem hiding this comment.
Did you investigate other types?
*bytes.Reader doesn't have it directly, but could be tricked into giving the backing bytes with WriteTo - which in it's current implementation will send all bytes as a single write. Seek can be used to forward.
*bytes.Buffer is less obvious. You can of course easily get the underlying bytes, but forwarding it cheaply without doing a Reset (which could break callers) seems more tricky.
Of course if ReadByte is inlined it is probably not worth pursuing.
The $GETBYTE_E$ placeholder started with a newline the template already supplied, so every read site picked up a blank line before its return -- 25 in total, and across all five variants rather than only the bufio one. With that fixed the four non-bufio variants are byte-identical to the stock generated output again (verified by diffing each generated function). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Yes — I benchmarked each reader type with a flate-style fill loop (concrete type per func, mirroring the codegen), MB/s:
The bytes types are already ~2× faster than bufio —
That's also why this PR stays on |
Problem
huffmanBufioReaderfills the bit buffer withfr.ReadByte()per input byte.bufio.Reader.ReadBytedoesn't inline, so each byte is a call barrier that spills the fill loop's registers — ~1.85× slower than the*bytes.Readervariant, whoseReadByteinlines to a slice index.Change
Read via
Peekinstead: index bufio's buffered slice directly (pbuf[pos]), refilling when drained.Peekonly views the buffer, so consumed bytes areDiscarded before every return, keeping the reader positioned fornextBlock/moreBits/the gzip trailer.peekBufiorefills from at most oneRead(notPeek(Size()), which would block a sync writer).Only the
*bufio.Readervariant changes (in_gen/gen_inflate.go); the other four stay byte-identical.Validation
-racepass on linux/amd64, linux/386, darwin/arm64.FuzzInflateBufio(forces the bufio path, byte-exact round-trip): ~110k execs, no mismatch.Numbers
*bufio.Readerdecode, 8 MiB literal-heavy (median of 3):Match-heavy data ~1.05–1.07× (huffman-table-load bound, unchanged).
Summary by CodeRabbit
Bug Fixes
Tests