Reserve buffer capacity before every encode_into write#3103
Merged
Conversation
Owner
Author
The optimized encode path writes primitives directly into a growable
EncodeBuffer (initial 64KB). Only Bytes reserved space via out.ensure();
every other writer assumed room existed. When a payload forces ensure()
to grow the buffer to exactly fit (zero trailing slack, i.e. when
pos + payload > 2 * capacity), the next write ran off the end:
- UnsignedVarInt32 / tagged-field count -> IndexError (the reported bug)
- fixed fields (e.g. the next partition's int32 index) -> struct.error
- arrays of fixed primitives ([]int32) overflow at 64KB with no Bytes
or tagged fields involved at all
- codegen kept a stale `buf` local after a delegated tagged-field
encode reallocated out.buf (silent data loss / crash); adding
ensure() to the varint made this fire routinely
Make capacity reservation uniform:
- runtime codecs (types.py, tagged_fields.py) call out.ensure(N) before
writing N bytes (fixed/uuid/bitfield/varint, String prefix+payload,
Bytes length prefix, tagged size byte)
- codegen gains CodegenContext.emit_reserve(): an inline
`if pos + N > _cap` check that only syncs/grows/re-reads on overflow,
keeping buf/pos/_cap in sync
- ArrayField bulk-reserves fixed-element arrays once, then writes the
run with a tight pack_into loop (keeps the hot path fast)
- struct.py / struct_array.py re-bind buf/_cap after every delegated
encode_into that can reallocate the buffer
Document the EncodeBuffer capacity contract and the reallocation hazard
(callers must re-read out.buf after a grow).
Adds test/protocol/schemas/test_encode_large.py: boundary unit tests for
every codec plus byte-for-byte parity and round-trip for large records
(Produce v9/v3, single and multi partition), large []int32 arrays, the
64KB/128KB grow boundaries, and pooled-buffer reuse. 22 of these fail on
the prior tree.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bdd31f4 to
3b7462f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The optimized encode path writes primitives directly into a growable
EncodeBuffer (initial 64KB). Only Bytes reserved space via out.ensure();
every other writer assumed room existed. When a payload forces ensure()
to grow the buffer to exactly fit (zero trailing slack, i.e. when
pos + payload > 2 * capacity), the next write ran off the end:
or tagged fields involved at all
buflocal after a delegated tagged-fieldencode reallocated out.buf (silent data loss / crash); adding
ensure() to the varint made this fire routinely
Make capacity reservation uniform:
writing N bytes (fixed/uuid/bitfield/varint, String prefix+payload,
Bytes length prefix, tagged size byte)
if pos + N > _capcheck that only syncs/grows/re-reads on overflow,keeping buf/pos/_cap in sync
run with a tight pack_into loop (keeps the hot path fast)
encode_into that can reallocate the buffer
Document the EncodeBuffer capacity contract and the reallocation hazard
(callers must re-read out.buf after a grow).
Adds test/protocol/schemas/test_encode_large.py: boundary unit tests for
every codec plus byte-for-byte parity and round-trip for large records
(Produce v9/v3, single and multi partition), large []int32 arrays, the
64KB/128KB grow boundaries, and pooled-buffer reuse. 22 of these fail on
the prior tree.
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com