(improvement) perf: remove copies on the read path (us improvements - x1.5-3.7 speedup!) - #734
(improvement) perf: remove copies on the read path (us improvements - x1.5-3.7 speedup!)#734mykaul wants to merge 5 commits into
Conversation
|
#630 would have helped with wide_5k_doubles of course. |
3e11f41 to
213c0e8
Compare
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Rebased onto current Since this PR's whole point is removing copies on the hot read path, I went through every copy elimination looking specifically for use-after-free/aliasing risk (a stale reference outliving the buffer it points into, or a reference to something that later gets mutated in place):
One real gap I did close (amended into CI: all required checks (Integration tests on libev/asyncio/asyncore × Python 3.11–3.14t, Docs build, security/snyk) are green on the latest push. "Test wheels building" shows red, but it's a Ran the full No open review threads to resolve. Still a draft as requested. |
There was a problem hiding this comment.
Pull request overview
Note
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.
This PR reduces memory copying along the driver read/decode path to improve latency and throughput, especially for large payloads, by replacing io.BytesIO-based reads with lighter and more zero-copy-friendly primitives.
Changes:
- Introduces
BytesReaderand wires it intoProtocolHandler.decode_message()to avoidio.BytesIO(body)copying. - Adds zero-copy handoff support to the Cython row parser via
BytesIOReader(..., offset)andremaining_buffer(). - Optimizes connection buffering/reset and adds unit tests plus an isolated decode benchmark script.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
cassandra/protocol.py |
Adds BytesReader and switches decode to use it instead of io.BytesIO. |
cassandra/row_parser.pyx |
Avoids f.read() copy by using (buffer, offset) handoff into BytesIOReader. |
cassandra/bytesio.pyx / cassandra/bytesio.pxd |
Adds offset support to BytesIOReader for suffix reads without copying. |
cassandra/connection.py |
Reduces copies by using getbuffer(), adds _reset_buffer, and bumps recv buffer size. |
tests/unit/test_protocol.py |
Adds unit tests for BytesReader and end-to-end decode_message coverage. |
tests/unit/test_connection.py |
Adds tests for _ConnectionIOBuffer._reset_buffer. |
tests/unit/test_bytesio_reader.py |
Adds Cython-only tests for BytesIOReader offset construction/validation. |
benchmarks/decode_benchmark.py |
Adds a standalone benchmark to measure decode performance across scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace getvalue() with getbuffer() memoryview in _read_frame_header and frame body extraction to avoid full-buffer copies. Add _reset_buffer() helper using getbuffer()[pos:] instead of read() to reduce allocations. Wrap memoryview usage in try/finally to ensure release before mutation. Increase in_buffer_size from 4096 to 16384 to reduce recv() call overhead.
Introduce a lightweight BytesReader class that operates directly on bytes data without BytesIO overhead. Materializes memoryview to bytes once in __init__ instead of checking on every read(). Includes remaining_buffer() method for zero-copy handoff to Cython parsers.
Add offset parameter to BytesIOReader so it can start reading from the middle of an existing buffer, avoiding the full-body copy at the Python-to-Cython boundary. Update row_parser.pyx to use f.remaining_buffer() for zero-copy handoff with hasattr fallback. Track _initial_offset for error recovery.
13 BytesReader tests covering read operations, remaining_buffer(), memoryview materialization, empty data, and EOFError handling. 9 BytesIOReader tests covering offset initialization, boundary conditions, read behavior with offset, and error cases.
…pact Standalone benchmark (no cluster required) that constructs synthetic RESULT/ROWS wire-format bodies and measures ProtocolHandler.decode_message() throughput across 8 scenarios (small to 16MB, narrow to 20-column wide). Pins to a single CPU core via sched_setaffinity for consistent results. Supports both Cython and pure-Python paths, with --cprofile option. Re-benchmarked against origin/master with this tool (isolated core, 25 iterations + 3 warmup): the originally-claimed 1.5-3.7x speedups do not reproduce for most scenarios. small_100, medium_1k_256B, wide_5k_doubles, and wide_1k_20cols show no measurable improvement (within noise, one even slightly slower); medium_1k_1KB, large_5k_1KB, and large_1k_4KB show a real but smaller 1.1-1.6x; only blob_1k_16KB matches/exceeds its claim (2.06x measured vs 1.8x claimed). See PR description for the full table. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
8eb9246 to
8a37148
Compare
This patch set aims to reduce the memory copies we perform in the read path, to improve overall performance - mainly reduce latency on the processing side of the driver receiving the payload.
This is more effective on larger payloads of course.
Re-benchmarked against master (same tool as below -
benchmarks/decode_benchmark.py, Cython path, isolated core, 25 iterations + 3 warmup). Most of the originally-claimed 1.5-3.7x speedups did not reproduce:Only
blob_1k_16KBmatches/exceeds its original claim. Four scenarios show no measurable improvement (or a slight regression); the remaining three show a real but smaller 1.1-1.6x. This is the same pattern found on PR #630 (Optimize column_encryption_policy checks in recv_results_rows) — claimed multipliers not holding under careful re-measurement in this environment. The original benchmark table below is retained for history but should not be relied on as-is.The correctness/robustness value of this PR (reducing allocations and copies on the read path, adding
BytesReadertest coverage) still stands independent of the exact multiplier.Original benchmark (see note above)
Comparison to master (Cython path, 10 iterations, CPU pinned, all times in microseconds):
(note - we can see that wide_5k_doubles is bottlenecked on something else - CPU processing of this payload - unpacking it. This may be optimized in a different PR)
./docs/source/.Fixes:annotations to PR description.🤖 Generated with Claude Code