code: unify BytesIO allocation in encode_message compression path (no measurable end-to-end perf effect) - #800
Conversation
Benchmark results (CPython 3.14, 500k iterations)
Applies to protocol v4 and below where compression is at the message level. Eliminates one |
f3f51d3 to
2b4e88b
Compare
Follow-up commit: inline has_checksumming_support checkCommit: ChangeReplaced Semantics are identical: Benchmark results (
|
04e4ba5 to
859ddfe
Compare
859ddfe to
469c78d
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
|
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 optimizes protocol frame encoding/decoding hot paths by reducing BytesIO allocations in the pre-v5 compression path and inlining the “checksumming supported” check to avoid classmethod overhead.
Changes:
- Reworked
encode_messageto use a singleio.BytesIO()for both compressed and non-compressed paths, returningheader + bodydirectly for compressed frames. - Inlined checksumming-support detection via module-level constants and integer comparisons in both
encode_messageanddecode_message. - Added a micro-benchmark to measure the overhead of the classmethod call vs inline comparison.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cassandra/protocol.py | Reduces allocations in encode_message compression path; replaces classmethod checksumming test with inline range check in encode/decode. |
| benchmarks/micro/bench_checksumming_inline.py | Adds a micro-benchmark to quantify the classmethod vs inline checksumming check overhead. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
469c78d to
473a1c9
Compare
473a1c9 to
1fc8656
Compare
…n path When compression is active (protocol v4 and below), encode_message previously created two BytesIO objects: one for the uncompressed body, then another to write the header + compressed body. The second BytesIO is unnecessary -- use direct bytes concatenation (header + compressed_body) instead, which avoids one BytesIO allocation per compressed message. The non-compression path already used a single BytesIO and is unchanged. Signed-off-by: Yaniv Michael Kaul <yaniv.kaul@scylladb.com>
… overhead Replace ProtocolVersion.has_checksumming_support(protocol_version) calls in encode_message and decode_message with inline integer comparisons using pre-computed module-level constants. This avoids the classmethod dispatch overhead on every encode/decode call. Benchmark: classmethod call: 61.3 ns inline compare: 25.5 ns saving: 35.8 ns/call (2.4x) Signed-off-by: Yaniv Michael Kaul <yaniv.kaul@scylladb.com>
1fc8656 to
d06ace8
Compare
Summary
Unify the two
BytesIOallocations inencode_messageinto a singlebuff = io.BytesIO()declared once before the compression branch. In the compression path (protocol v4 and below), the body is written intobuff, extracted viagetvalue(), compressed, and returned via direct bytes concatenation (header + compressed_body). The non-compression path reuses the samebuffwith the existing seek-based header reservation pattern.Before (master): 2
BytesIO()allocations on the compression path (buff+body), 1 on the non-compression path.After: 1
BytesIO()allocation on both paths.Motivation
For protocol v4 and below, compression happens at the message level (v5+ uses segment-level compression with checksumming). The original code created a separate
body = io.BytesIO()for the uncompressed payload, then copied the result into a secondbuff = io.BytesIO()before writing the header. This second buffer is unnecessary — we can write the body intobuffdirectly, extract it, compress, and concatenate the header as bytes.Benchmark (updated — see note below)
Original PR description quoted a two-run, per-body-size microbenchmark showing 17-32% savings. That measurement did not hold up under closer scrutiny.
A more rigorous re-measurement (
_ProtocolHandler.encode_messageend-to-end,RegisterMessage+ lz4 compressor, protocol v4, 10 independent fresh-process repeats per branch, each the median of 7 timeit reps over 1M iterations, run back-to-back on an otherwise idle machine) found:Delta: ~3ns / 0.24% — not statistically distinguishable from noise (both distributions fully overlap, well inside each side's own stddev). An earlier single-process, 3-repeat pass had shown ~150ns/~10%, but that appears to have been an artifact of run ordering/warm-up rather than a real effect — it did not replicate.
Removing one
BytesIO()allocation is a real, structurally sound simplification, but at this call's scale the fixed costs (lz4 compression, string-list encoding, dict overhead) dominate and the saved allocation isn't measurable in the overallencode_messagecost. Landing this on code-quality/simplicity grounds rather than a performance claim.Changes
cassandra/protocol.py: Movebuff = io.BytesIO()before theifbranch. In the compression path, write body intobuffinstead of a separatebody = io.BytesIO(), extract viagetvalue(), compress, and returnheader + bodyvia bytes concat. Non-compression path uses the samebuffwithseek(9)header reservation as before.Testing
Unit tests pass (645 passed, 43 skipped). The non-compression path is structurally unchanged.
Isolated allocation cost (supplementary)
In a microbenchmark isolating only the header-pack + body-write logic (no compressor in the loop), removing the second
BytesIO()saves ~159-185ns (~51-59%) per call. This confirms the allocation itself is meaningfully cheaper; it's just not visible at the fullencode_messagescale where lz4 compression dominates, per the end-to-end measurement above.