THRIFT-6069: python: add fastbinary decode_binary_from_bytes#3594
Open
markjm wants to merge 1 commit into
Open
THRIFT-6069: python: add fastbinary decode_binary_from_bytes#3594markjm wants to merge 1 commit into
markjm wants to merge 1 commit into
Conversation
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.
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 👎. |
Member
|
Hi, any plans to continue here? |
Contributor
Author
|
@Jens-G hi yes! Life took me away from code for a bit, but I'll get all the feedback addressed and PRs cleaned up for all the PRs I opened early/mid next week! Apologies for the delay |
Client: py 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. Use an overflow-safe remaining-bytes comparison in the direct bytes decode path so the position-plus-length check cannot wrap on extreme size_t values. 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. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
markjm
force-pushed
the
apache-port-fastbinary-decode-from-bytes
branch
from
6af2e54 to
2ca8447
Compare
Comment on lines
+142
to
+164
| static PyObject* decode_binary_from_bytes(PyObject*, PyObject* args) { | ||
| PyObject* bytes_obj = nullptr; | ||
| PyObject* typeargs = nullptr; | ||
| if (!PyArg_ParseTuple(args, "OO", &bytes_obj, &typeargs)) { | ||
| return nullptr; | ||
| } | ||
| if (!PyBytes_Check(bytes_obj)) { | ||
| PyErr_SetString(PyExc_TypeError, "first argument must be bytes"); | ||
| return nullptr; | ||
| } | ||
|
|
||
| StructTypeArgs parsedargs; | ||
| if (!parse_struct_args(&parsedargs, typeargs)) { | ||
| return nullptr; | ||
| } | ||
|
|
||
| BinaryProtocol protocol; | ||
| if (!protocol.prepareDecodeBufferFromBytes(bytes_obj)) { | ||
| return nullptr; | ||
| } | ||
|
|
||
| return protocol.readStruct(Py_None, parsedargs.klass, parsedargs.spec); | ||
| } |
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.