Fix #1622: size base64 encoding buffer from binary length hint - #1624
Conversation
When writing binary content from an InputStream of known length via `writeBinary(Base64Variant, InputStream, int)`, the read/encoding buffer was always the small default (2000 bytes), forcing many InputStream reads for large content (and, for sources like protobuf streams, an extra throwaway in-memory copy when the supplied buffer is smaller than the serialized size). Now, when the length is known (> 0), the buffer is sized from that hint, capped at 64kB to bound retention of ThreadLocal-recycled buffers. Applied to both UTF8JsonGenerator and WriterBasedJsonGenerator. The default base64 codec buffer is also bumped from 2000 to 16000 bytes to help the unknown-length path. Per maintainer guidance on the issue. Verification: added BinaryWriteBufferSize1622Test asserting the read buffer is sized to the (capped) hint and that output still round-trips; existing base64 and buffer-recycler tests pass.
| // reads when encoding binary content of unknown/large length. | ||
|
|
||
| private final static int[] BYTE_BUFFER_LENGTHS = new int[] { 8000, 8000, 2000, 2000 }; | ||
| private final static int[] BYTE_BUFFER_LENGTHS = new int[] { 8000, 8000, 2000, 16000 }; |
There was a problem hiding this comment.
I don't know the answer here but is there a reason not to keep this ordered? 8000 > 2000 so maybe the 16000 should go first.
There was a problem hiding this comment.
Good question. This array isn't sorted by value — it's positional: each slot is addressed by the named index constants just above it, and byteBufferLength(int ix) returns BYTE_BUFFER_LENGTHS[ix]. So the entries are in index order:
[0]BYTE_READ_IO_BUFFER= 8000[1]BYTE_WRITE_ENCODING_BUFFER= 8000[2]BYTE_WRITE_CONCAT_BUFFER= 2000[3]BYTE_BASE64_CODEC_BUFFER= 16000 ← the slot this PR bumps
The 16000 lands at the end only because the base64 codec buffer happens to be index 3; reordering the array would silently remap every buffer's length to the wrong purpose. Happy to add a brief inline comment naming each slot if you think that'd make the positional intent clearer.
There was a problem hiding this comment.
Yeah these must not be ordered by size, they are indexed by position as @seonwooj0810 pointed out.
However, I realized something: this will also affect parser-side, if changed; Base64-decoding buffer (in addition to generator-size encoding buffer).
And in fact, not quite sure default really needs changing: if and when actual size is indicated, we'll be using that anyway. I think I'll change default to 4000 as compromise.
3 participants
Fixes #1622
Root cause
writeBinary(Base64Variant, InputStream, int dataLength)always allocated the read/encoding buffer at the small default size (2000 bytes) via_ioContext.allocBase64Buffer(), ignoring the supplieddataLength. For large binary content this forces manyInputStreamreads, and for sources like a protobufInputStream(where aread(byte[])smaller than the serialized size triggers an extra throwaway in-memory copy) it adds avoidable allocation.Change
dataLength > 0, size the buffer from that hint:allocBase64Buffer(min(dataLength, MAX_BASE64_ENCODE_BUFFER_LENGTH)). The cap (64kB) bounds retention ofThreadLocal-recycled buffers, addressing the memory-retention concern raised in the issue.UTF8JsonGeneratorandWriterBasedJsonGenerator(identical pattern).BufferRecyclerfrom 2000 → 16000 bytes, which also helps the unknown-length path.This follows your guidance in the issue ("passes size hint, capped at say 64kb … based on
dataLengthif > 0" and "increasing default from 2kb to … 16kB"). One PR against2.xas suggested, for roll-forward to 3.x.Tests
Added
BinaryWriteBufferSize1622Test: a recordingInputStreamconfirms the generator now requests reads sized to the (capped) hint — 50,000 for a 50k payload and exactly 64kB for a 200k payload — for both byte- and char-backed generators, while the produced base64 still round-trips back to the original bytes. Without the fix the buffer stays at the default (≤16k), so these assertions fail.Verification done:
.javagenerator/recycler code plus a test.2.x(allocBase64Buffer()with no size hint)../mvnw test -Dtest=BinaryWriteBufferSize1622Test,Base64GenerationTest,Base64BinaryParsingTestand theutil/iopackages all pass (incl.BufferRecyclertests).AI-assistance disclosure: implemented with the help of Claude Code (consistent with prior
w/ Claude codecredits in this repo).