THRIFT-6069: python: add fastbinary decode_binary_from_bytes#3594
Open
markjm wants to merge 2 commits into
Open
THRIFT-6069: python: add fastbinary decode_binary_from_bytes#3594markjm wants to merge 2 commits into
markjm wants to merge 2 commits into
Conversation
Add a bytes-based fastbinary decode entry point so callers that already hold serialized thrift data can bypass TMemoryBuffer and protocol wrapper setup while reusing the existing struct decoder. Performance (50k iterations, warmed) | Workload | decode_binary (transport) | decode_binary_from_bytes | Speedup | |----------|---------------------------|--------------------------|---------| | simple (30B) | 3.59 us | 0.81 us | 4.43x | | 10-string (182B) | 9.52 us | 6.77 us | 1.41x | | complex (395B) | 12.03 us | 8.26 us | 1.46x | The transport overhead (TMemoryBuffer + TBinaryProtocolAccelerated + BytesIO) is a fixed ~2.6us per call. For small structs this dominates; for larger structs the per-field decode work catches up.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new fastbinary entrypoint to decode Thrift binary structs directly from a bytes object (bypassing transports), and validates behavior through Python tests.
Changes:
- Add
decode_binary_from_bytes(bytes, typeargs)to the Python C extension module. - Extend the C++ decode buffer to support direct reads from an in-memory
bytesbuffer. - Add Python tests (including accelerated protocol parity + error cases) using a simple Thrift-like struct.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/py/test/thrift_TBinaryProtocol.py | Adds tests and a minimal struct to validate decoding from raw bytes vs transport-based decoding. |
| lib/py/src/ext/types.h | Extends DecodeBuffer to store and track a direct bytes source and cursor. |
| lib/py/src/ext/protocol.tcc | Implements direct-buffer reads and adds a new prepareDecodeBufferFromBytes initializer. |
| lib/py/src/ext/protocol.h | Declares prepareDecodeBufferFromBytes on the protocol base class. |
| lib/py/src/ext/module.cpp | Exposes decode_binary_from_bytes to Python and wires it into the module method table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+232
to
+240
| @classmethod | ||
| def read(cls, iprot): | ||
| if ( | ||
| iprot._fast_decode is not None | ||
| and isinstance(iprot.trans, TTransport.CReadableTransport) | ||
| and cls.thrift_spec is not None | ||
| ): | ||
| return iprot._fast_decode(None, iprot, [cls, cls.thrift_spec]) | ||
| return iprot.readStruct(cls, cls.thrift_spec, False) |
Comment on lines
+305
to
+314
| if (input_.direct_buf) { | ||
| if (input_.direct_pos + static_cast<size_t>(len) > input_.direct_size) { | ||
| PyErr_SetString(PyExc_EOFError, "read past end of buffer"); | ||
| return false; | ||
| } | ||
|
|
||
| *output = const_cast<char*>(input_.direct_buf + input_.direct_pos); | ||
| input_.direct_pos += static_cast<size_t>(len); | ||
| return true; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
good catch mr bot - fixed
Comment on lines
166
to
173
| static PyMethodDef ThriftFastBinaryMethods[] = { | ||
| {"encode_binary", encode_binary, METH_VARARGS, ""}, | ||
| {"decode_binary", decode_binary, METH_VARARGS, ""}, | ||
| {"encode_compact", encode_compact, METH_VARARGS, ""}, | ||
| {"decode_compact", decode_compact, METH_VARARGS, ""}, | ||
| {"decode_binary_from_bytes", decode_binary_from_bytes, METH_VARARGS, ""}, | ||
| {nullptr, nullptr, 0, nullptr} /* Sentinel */ | ||
| }; |
Contributor
Author
There was a problem hiding this comment.
following whats already here
Use an overflow-safe remaining-bytes comparison in the direct bytes decode path so extreme size_t values cannot wrap the position-plus-length check and bypass EOF enforcement.
markjm
force-pushed
the
apache-port-fastbinary-decode-from-bytes
branch
from
3af096a to
6af2e54
Compare
markjm
changed the title
Jens-G
changed the title
Member
Code reviewFound 2 issues:
Lines 57 to 71 in 35c1a53 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
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.
Hi - I figured I'd share a few perf optimizations we are using internally. We are still on an older thrift version (😢 ), so I did use some agent magic to port these to the head of this repo. My testing was primarily on my branch, so buyer beware on that front!
This one is 1 of 3 PRs
Add a bytes-based fastbinary decode entry point so callers that already hold serialized thrift data can bypass TMemoryBuffer and protocol wrapper setup while reusing the existing struct decoder.
Performance (50k iterations, warmed)
The transport overhead (TMemoryBuffer + TBinaryProtocolAccelerated + BytesIO) is a fixed ~2.6us per call. For small structs this dominates; for larger structs the per-field decode work catches up.