THRIFT-6436: Avoid repeated bytes concatenation in Python readAll by OneSizeFitsQuorum · Pull Request #4028 · apache/thrift

OneSizeFitsQuorum · 2026-10-20T13:06:15Z

JIRA: https://issues.apache.org/jira/browse/THRIFT-6436
Client: py

TTransportBase.readAll repeatedly copies the accumulated bytes when a read completes through multiple short reads. Collect chunks and join them once instead. A short inline comment explains why chunks are joined once.

The patch is limited to the accumulation loop and seven regression tests in the existing transport test file. They cover fragmented and single reads, remaining request sizes, nonpositive sizes, EOF, exception propagation, consecutive reads, and a fragmented frame.

Local performance validation

A separate Python TSimpleServer process returned a prebuilt 30 MiB binary payload over loopback TCP, using TBinaryProtocol and TFramedTransport. Only the client's readAll implementation differed. Each implementation had seven alternating rounds of three measured RPCs, with one warm-up per round. Values below are medians over 21 RPCs; timing excludes connection setup and payload verification. Frame/string limits were explicitly configured for the large test response.

Environment: CPython 3.10.12, Linux 6.8.0-136-generic x86_64, Intel Xeon Platinum 8362 @ 2.80 GHz, eight logical CPUs visible. Baseline: db8caa7.

Socket read mode	RPC wall time, before → after	Client CPU time, before → after	Payload throughput, before → after
Unrestricted	76.44 → 50.96 ms	39.56 → 14.88 ms	392.5 → 588.7 MiB/s
Requests capped at 64 KiB	732.53 → 49.96 ms	695.42 → 15.30 ms	41.0 → 600.5 MiB/s

Unrestricted frame-body read counts ranged from 11–14 before and 136–437 after, so fragmentation was not identical. The explicitly controlled 64 KiB case used exactly 481 body reads for both implementations. These are local synthetic-service measurements, not HBase throughput results or predictions for other deployments. The RPC harness and raw results remain local.

Standalone reproduction of fragmented-read cost

This is an in-memory check of the accumulation cost, independent of the TCP measurements above. After checking out this PR:

(cd lib/py && python3 setup.py build)
git show HEAD^:lib/py/src/transport/TTransport.py > /tmp/readall-before.py
PYTHONPATH=lib/py/test python3 - <<'PY'
import importlib.util
import statistics
import timeit

import _import_local_thrift
from thrift.transport.TTransport import TTransportBase

spec = importlib.util.spec_from_file_location('baseline', '/tmp/readall-before.py')
baseline = importlib.util.module_from_spec(spec)
spec.loader.exec_module(baseline)
data = bytes(range(256)) * (30 * 1024 * 1024 // 256)
width = (len(data) + 54) // 55
chunks = [data[i:i + width] for i in range(0, len(data), width)]


class Fragmented(TTransportBase):
    def __init__(self):
        self.chunks = iter(chunks)

    def read(self, size):
        return next(self.chunks, b'')


for label, implementation in (
    ('before', baseline.TTransportBase.readAll), ('after', TTransportBase.readAll)
):
    assert implementation(Fragmented(), len(data)) == data
    times = timeit.repeat(lambda: implementation(Fragmented(), len(data)), number=1, repeat=7)
    print(label, round(statistics.median(times) * 1000, 2), 'ms')
PY

Validation

The seven new regression tests were added and run against the original implementation before applying the runtime change.
All 14 transport tests passed after the change, along with the local TCP A/B measurements above.
All 26 Python-specific CI checks passed for the final commit, covering Python 3.10–3.14 on Linux and the macOS matrix, plus Python static analysis.
flake8 --max-line-length=100, codespell and git diff --check passed for the final patch.
make style is unavailable in this unconfigured checkout because there is no root Makefile.
CI environment note: the Smalltalk setup job fails while downloading the Pharo image (curl exit 7), before its tests run. Repository administrator permissions are required to rerun that job.

Generated-by: OpenAI Codex (GPT-6)

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

3 open issues

struct.pack is used in the new test but there is no import struct shown in the updated imports.… · New
The benchmark assumes only the 4-byte frame header uses readAll(4) and treats any `readAll(sz <=… · New
spec_from_file_location() can return None, and spec.loader can be None (e.g., if… · New

What changed in this PR

Improves Python client transport performance by making TTransportBase.readAll accumulate chunks and join() once, avoiding repeated bytes concatenation for fragmented reads.

Changes:

Refactor TTransportBase.readAll to collect read chunks in a list and b''.join() at the end.
Add regression tests for readAll behavior across fragmentation, EOF, exceptions, and framed payloads.
Add a loopback RPC benchmark to compare baseline vs updated readAll behavior and record socket read fragmentation.

File	Description
lib/py/src/transport/TTransport.py	Switch `readAll` from incremental bytes concatenation to list accumulation + join for better performance on fragmented reads.
lib/py/test/thrift_transport.py	Add regression tests and a small fragmented transport helper to verify `readAll` semantics.
lib/py/test/benchmark_readall.py	Add a standalone benchmark to measure before/after `readAll` behavior over a framed RPC on loopback TCP.

_{Review effort: Lite}

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

6 open issues

test_benchmark_readall.py (and benchmark_readall.py) uses Python 3-only stdlib features (e.g.,… · New
test_benchmark_readall.py (and benchmark_readall.py) uses Python 3-only stdlib features (e.g.,… · New
struct.pack is used in the new test but there is no import struct shown in the updated imports.…
server.kill() is not available on older Python 3 versions (it was added after the earliest 3.x… · New
spec_from_file_location() can return None, and spec.loader can be None (e.g., if…
The benchmark assumes only the 4-byte frame header uses readAll(4) and treats any `readAll(sz <=…

_{Review effort: Lite}

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

7 open issues

test_benchmark_readall.py (and benchmark_readall.py) uses Python 3-only stdlib features (e.g.,…
test_benchmark_readall.py (and benchmark_readall.py) uses Python 3-only stdlib features (e.g.,…
struct.pack is used in the new test but there is no import struct shown in the updated imports.…
This changes the accepted element types in readAll: b''.join(...) accepts general bytes-like… · New
server.kill() is not available on older Python 3 versions (it was added after the earliest 3.x…
spec_from_file_location() can return None, and spec.loader can be None (e.g., if…
The benchmark assumes only the 4-byte frame header uses readAll(4) and treats any `readAll(sz <=…

_{Review effort: Lite}

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

10 open issues

struct.pack is used but this diff doesn't add an import struct in the module imports shown… · New
test_benchmark_readall.py (and benchmark_readall.py) uses Python 3-only stdlib features (e.g.,…
test_benchmark_readall.py (and benchmark_readall.py) uses Python 3-only stdlib features (e.g.,…
struct.pack is used in the new test but there is no import struct shown in the updated imports.…
This changes the accepted element types in readAll: b''.join(...) accepts general bytes-like…
server.kill() is not available on older Python 3 versions (it was added after the earliest 3.x…
spec_from_file_location() can return None, and spec.loader can be None (e.g., if…
The benchmark assumes only the 4-byte frame header uses readAll(4) and treats any `readAll(sz <=…
The new comment states a behavioral requirement about mutation, but read() in Python transports… · New
On EOF (chunkLen == 0), the code appends an empty chunk to buff and then immediately raises.… · New

_{Review effort: Lite}

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

2 open issues

The inline comment at line 86 is misleading where it sits (inside the loop) because the join… · New
self.assertIs(type(result), bytes) is an unidiomatic type assertion in unittest. Prefer… · New

10 resolved since last review

_{Review effort: Lite}

Client: py Collect readAll chunks and join once, preserving short-read and EOF behavior. Add regression tests for fragmented reads, read sizes, EOF and exception propagation. Generated-by: OpenAI Codex (GPT-6)

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

No open issues 🎉

2 resolved since last review

self.assertIs(type(result), bytes) is an unidiomatic type assertion in unittest. Prefer…
The inline comment at line 86 is misleading where it sits (inside the loop) because the join…

_{Review effort: Lite}

Jens-G · 2026-10-22T22:23:40Z

Code review

No blocking issues found. Checked for bugs and CLAUDE.md compliance.

Three suggestions, below the bar for an issue but verified against the merge-base (Python 3.10 and 3.12):

readAll now keeps every chunk until the join, so what a large read holds grows with the number of reads rather than with the bytes received. TSocket.read() asks recv() for the whole remainder, and with glibc each short result keeps a 4 KiB page. With TFramedTransport over TSocket, a declared 16,384,000-byte frame and 2 bytes per recv(), the reader's RSS grew by 78 MiB after 20,000 reads, against 0.1-0.2 MiB before this change.
The common case, where the first read() already returns everything, got slower: b'' + chunk returned the chunk itself, while the list, append and join cost more. The pure-Python protocols call readAll for every value, so 200,000 readI32 calls through TBinaryProtocol over TMemoryBuffer now take 15-20% longer (3.12: about 91 to 108 ms, 3.10: about 121 to 140 ms).

Returning a complete first chunk directly and otherwise accumulating into a bytearray, as TNonblockingServer already does, covers both. The trickle case then stays at 0.1-0.2 MiB, readI32 takes 80 ms (3.12) and 109 ms (3.10), 8 MiB in 64 KiB pieces takes about 3.3 ms (2 ms with this PR's list, 78 ms before), and the 14 transport tests pass:
```
def readAll(self, sz):
    if sz <= 0:
        return b''
    chunk = self.read(sz)
    have = len(chunk)
    if have == 0:
        raise EOFError()
    if have >= sz:
        return b'' + chunk
    # A bytearray grows in place and drops each chunk once it is copied.
    buff = bytearray(chunk)
    while (have < sz):
        chunk = self.read(sz - have)
        chunkLen = len(chunk)
        if chunkLen == 0:
            raise EOFError()
        have += chunkLen
        buff += chunk

    return bytes(buff)
```

thrift/lib/py/src/transport/TTransport.py

Lines 76 to 90 in c251c26

    
           def readAll(self, sz): 
        
               # Collect chunks and join once to avoid repeated prefix copies. 
        
               buff = [] 
        
               have = 0 
        
               while (have < sz): 
        
                   chunk = self.read(sz - have) 
        
                   chunkLen = len(chunk) 
        
                   if chunkLen == 0: 
        
                       raise EOFError() 
        
                   have += chunkLen 
        
                   buff.append(chunk) 
        
               return b''.join(buff)

The seven new tests also pass against the old readAll, so none of them would notice a return to +=. A copy-counting test like the one in test_nonblocking_server_read_buffer.py (THRIFT-6267) would: a bytes subclass whose __radd__ counts what it copies, 1 MiB read in 4 KiB pieces, at most twice the payload allowed. It fails before this change (134,742,016 bytes copied) and passes with it (0), and with the variant above (0).

thrift/lib/py/test/thrift_transport.py

Lines 47 to 99 in c251c26

    
           class TestReadAll(unittest.TestCase): 
        
               def test_fragmented_read(self): 
        
                   chunks = [b'\x00\xff', b'a', b'bcd'] 
        
                   transport = FragmentedTransport(chunks) 
        
                   self.assertEqual(transport.readAll(6), b''.join(chunks)) 
        
                   self.assertEqual(transport.requests, [6, 4, 3]) 
        
               def test_single_read(self): 
        
                   transport = FragmentedTransport([b'abc']) 
        
                   result = transport.readAll(3) 
        
                   self.assertEqual(result, b'abc') 
        
                   self.assertIsInstance(result, bytes) 
        
                   self.assertEqual(transport.requests, [3]) 
        
               def test_nonpositive_size(self): 
        
                   for size in (0, -1): 
        
                       with self.subTest(size=size): 
        
                           transport = FragmentedTransport([]) 
        
                           self.assertEqual(transport.readAll(size), b'') 
        
                           self.assertEqual(transport.requests, []) 
        
               def test_eof(self): 
        
                   cases = (([], [4]), ([b'ab'], [4, 2]), ([b'ab', b'', b'cd'], [4, 2])) 
        
                   for chunks, requests in cases: 
        
                       with self.subTest(chunks=chunks): 
        
                           transport = FragmentedTransport(chunks) 
        
                           with self.assertRaises(EOFError): 
        
                               transport.readAll(4) 
        
                           self.assertEqual(transport.requests, requests) 
        
               def test_read_exception(self): 
        
                   error = TTransportException(TTransportException.TIMED_OUT, 'read timeout') 
        
                   transport = TTransport.TTransportBase() 
        
                   transport.read = Mock(side_effect=[b'ab', error]) 
        
                   with self.assertRaises(TTransportException) as caught: 
        
                       transport.readAll(4) 
        
                   self.assertIs(caught.exception, error) 
        
                   self.assertEqual(transport.read.call_count, 2) 
        
               def test_leaves_following_data_unread(self): 
        
                   transport = FragmentedTransport([b'ab', b'cd']) 
        
                   self.assertEqual(transport.readAll(2), b'ab') 
        
                   self.assertEqual(transport.readAll(2), b'cd') 
        
               def test_fragmented_frame(self): 
        
                   payload = bytes(range(256)) * 256 
        
                   chunks = [struct.pack('!i', len(payload))] 
        
                   chunks.extend(payload[i:i + 1024] for i in range(0, len(payload), 1024)) 
        
                   transport = TTransport.TFramedTransport(FragmentedTransport(chunks)) 
        
                   self.assertEqual(transport.readAll(len(payload)), payload)

🤖 Generated with Claude Code

OneSizeFitsQuorum requested a review from mhlakhani as a code owner October 20, 2026 13:06

Copilot AI balanced review requested due to automatic review settings October 20, 2026 13:06

mergeable Bot added the python label Oct 20, 2026

Copilot AI reviewed Oct 20, 2026

View reviewed changes

Comment thread lib/py/test/thrift_transport.py

Comment thread lib/py/test/benchmark_readall.py Outdated

Comment thread lib/py/test/benchmark_readall.py Outdated

Copilot stopped reviewing on behalf of OneSizeFitsQuorum due to an error October 20, 2026 17:36
Workflow completed but conclusion is not available. This may indicate rate limiting or API issues.

Copilot AI balanced review requested due to automatic review settings October 21, 2026 06:17

OneSizeFitsQuorum force-pushed the THRIFT-6436 branch from fac7037 to c4ceabf Compare October 21, 2026 06:17

mergeable Bot added the build and general CI cmake, automake and build system changes label Oct 21, 2026

Copilot AI reviewed Oct 21, 2026

View reviewed changes

Comment thread lib/py/Makefile.am Outdated

Comment thread lib/py/Makefile.am Outdated

Comment thread lib/py/test/benchmark_readall.py Outdated

Copilot stopped reviewing on behalf of OneSizeFitsQuorum due to an error October 21, 2026 06:45
The job was not acquired by a runner even after multiple attempts.

Copilot AI balanced review requested due to automatic review settings October 21, 2026 08:30

OneSizeFitsQuorum force-pushed the THRIFT-6436 branch from c4ceabf to a56da09 Compare October 21, 2026 08:30

Copilot AI reviewed Oct 21, 2026

View reviewed changes

Comment thread lib/py/src/transport/TTransport.py

Copilot stopped reviewing on behalf of OneSizeFitsQuorum due to an error October 21, 2026 09:43
The job was not acquired by a runner even after multiple attempts.

Copilot AI balanced review requested due to automatic review settings October 22, 2026 05:36

OneSizeFitsQuorum force-pushed the THRIFT-6436 branch from a56da09 to 443b9a7 Compare October 22, 2026 05:36

Copilot AI reviewed Oct 22, 2026

View reviewed changes

Comment thread lib/py/test/thrift_transport.py

Comment thread lib/py/src/transport/TTransport.py Outdated

Comment thread lib/py/src/transport/TTransport.py Outdated

Copilot AI balanced review requested due to automatic review settings October 22, 2026 06:44

OneSizeFitsQuorum force-pushed the THRIFT-6436 branch from 443b9a7 to a17e73f Compare October 22, 2026 06:44

Copilot stopped reviewing on behalf of OneSizeFitsQuorum due to an error October 22, 2026 06:59
The job was not acquired by a runner even after multiple attempts.

Copilot AI reviewed Oct 22, 2026

View reviewed changes

Comment thread lib/py/src/transport/TTransport.py

Comment thread lib/py/test/thrift_transport.py Outdated

THRIFT-6436: Avoid repeated bytes concatenation in Python readAll

c251c26

Client: py Collect readAll chunks and join once, preserving short-read and EOF behavior. Add regression tests for fragmented reads, read sizes, EOF and exception propagation. Generated-by: OpenAI Codex (GPT-6)

OneSizeFitsQuorum force-pushed the THRIFT-6436 branch from a17e73f to c251c26 Compare October 22, 2026 07:02

Copilot AI balanced review requested due to automatic review settings October 22, 2026 07:02

Copilot AI reviewed Oct 22, 2026

View reviewed changes

Copilot stopped reviewing on behalf of OneSizeFitsQuorum due to an error October 22, 2026 11:11
The job was not acquired by a runner even after multiple attempts.

Copilot stopped reviewing on behalf of OneSizeFitsQuorum due to an error October 22, 2026 11:49
The job was not acquired by a runner even after multiple attempts.

Conversation

OneSizeFitsQuorum commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Local performance validation

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Jens-G commented Oct 22, 2026

Code review

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

OneSizeFitsQuorum commented Oct 20, 2026 •

edited

Loading