THRIFT-6069: python: use a flat fastbinary encode buffer#3596
Open
markjm wants to merge 2 commits into
Open
Conversation
Replace std::vector back_inserter writes in the Python 3 fastbinary encoder with a malloc/realloc buffer so encoded bytes can be copied in bulk while preserving the existing growth behavior. Performance (50k iterations, warmed) | Workload | Baseline | This commit | Speedup | |----------|----------|-------------|---------| | encode simple (30B) | 0.60 us | 0.53 us | 1.13x | | encode 10-string (182B) | 1.44 us | 1.25 us | 1.15x | | encode complex (395B) | 3.02 us | 2.63 us | 1.15x | Larger object encode throughput | Message size | apache/master | branch | speedup | |--------------|---------------|--------|---------| | 8 KiB | 12.19 us | 1.11 us | 11.0x | | 64 KiB | 101.58 us | 5.48 us | 18.5x | | 256 KiB | 564.34 us | 25.22 us | 22.4x | | 1 MiB | 2188.97 us | 136.22 us | 16.1x | Decode performance is unchanged because this only affects the encode path.
markjm
changed the title
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds coverage for Thrift’s accelerated binary protocol round-tripping a large TApplicationException, and refactors the Python C-extension encode buffer implementation away from std::vector to a manual malloc/realloc-backed buffer.
Changes:
- Add a unit test that exercises
_fast_encode/_fast_decodeviaTBinaryProtocolAcceleratedFactoryfor a large exception payload. - Replace
EncodeBuffer’sstd::vector<char>storage with a manual heap buffer and update protocol write logic to usememcpy. - Add required C/C++ headers for the new buffer implementation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lib/py/test/thrift_TBinaryProtocol.py | Adds accelerated large-message roundtrip test coverage for the fastbinary path. |
| lib/py/src/ext/types.h | Refactors EncodeBuffer to raw malloc/realloc storage with destructor-based cleanup. |
| lib/py/src/ext/protocol.tcc | Updates encode buffer allocation and write path to use the new EncodeBuffer API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
134
to
+145
| struct EncodeBuffer { | ||
| std::vector<char> buf; | ||
| size_t pos; | ||
| char* data; | ||
| size_t size; | ||
| size_t capacity; | ||
|
|
||
| EncodeBuffer() : data(nullptr), size(0), capacity(0) {} | ||
|
|
||
| ~EncodeBuffer() { | ||
| if (data) { | ||
| free(data); | ||
| } | ||
| } |
Comment on lines
+157
to
+176
| bool ensure(size_t additional) { | ||
| size_t needed = size + additional; | ||
| if (needed <= capacity) { | ||
| return true; | ||
| } | ||
|
|
||
| size_t new_capacity = capacity == 0 ? needed : capacity; | ||
| while (new_capacity < needed) { | ||
| new_capacity *= 2; | ||
| } | ||
|
|
||
| char* new_data = static_cast<char*>(realloc(data, new_capacity)); | ||
| if (!new_data) { | ||
| return false; | ||
| } | ||
|
|
||
| data = new_data; | ||
| capacity = new_capacity; | ||
| return true; | ||
| } |
Comment on lines
+147
to
+155
| bool init(size_t initial_capacity) { | ||
| data = static_cast<char*>(malloc(initial_capacity)); | ||
| if (!data) { | ||
| return false; | ||
| } | ||
| size = 0; | ||
| capacity = initial_capacity; | ||
| return true; | ||
| } |
Comment on lines
+172
to
+179
| APPLICATION_EXCEPTION_TYPEARGS = [ | ||
| TApplicationException, | ||
| ( | ||
| None, | ||
| (1, 11, "message", "UTF8", None), | ||
| (2, 8, "type", None, None), | ||
| ), | ||
| ] |
Make EncodeBuffer explicitly non-copyable, handle zero-capacity initialization, guard capacity growth against size_t overflow, and tighten the large-message fastbinary test to use immutable Thrift spec metadata.
Member
As long as they adhere to AGENTS.md all is fine. |
Jens-G
changed the title
Member
Code reviewFound 1 issue:
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
Replace std::vector back_inserter writes in the fastbinary encoder with a malloc/realloc buffer so encoded bytes can be copied in bulk while preserving the existing growth behavior.
Performance (50k iterations, warmed)
Decode performance is unchanged because this only affects the encode path.