perf: add Cython metadata parser using BytesIOReader (100's-1000's of ns improvements, x1.5-4 speedup) - #814
Conversation
48538ac to
3ec7841
Compare
|
I'm slowly but surely converting the Python driver to a C driver.... Unsure about this direction. :-/ |
mykaul
changed the title
mykaul
changed the title
713bcc5 to
1f3e38f
Compare
1f3e38f to
5854f6a
Compare
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Rebased onto current Consistency with #745 (schema-parsing SELECT trimming / Buffer-bounds safety: found and fixed a real gap in Circular-import anti-pattern: Testing: rebuilt Cython extensions from a clean All changes amended into the existing commit (no new commits added); force-pushed with |
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 introduces a Cython BytesIOReader-based metadata parser to reduce per-read allocations during RESULT_KIND_ROWS decoding, improving the hot-path performance for result metadata parsing.
Changes:
- Added
cassandra/metadata_parser.pyximplementing zero-copyrecv_results_metadataand type parsing onBytesIOReader - Updated
cassandra/row_parser.pyxto reuse a singleBytesIOReaderfor both metadata+rows and adjust error recovery - Updated
cassandra/protocol.pyto wire the new metadata parser into the Cython protocol handler; added unit tests for the new parser
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 |
|---|---|
cassandra/metadata_parser.pyx |
New Cython metadata/type parser using BytesIOReader to avoid per-read bytes allocations |
cassandra/row_parser.pyx |
Reuses a single BytesIOReader across metadata and row parsing; adjusts exception rewind logic |
cassandra/protocol.py |
Constructs and injects the new metadata parser closure into make_recv_results_rows() |
cassandra/ioutils.pyx |
Adds a Cython read_short(BytesIOReader) primitive used by the new parser |
tests/unit/cython/test_metadata_parser.py |
Adds correctness and malformed-frame tests for the new metadata parser |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add cassandra/metadata_parser.pyx with zero-copy metadata parsing that uses BytesIOReader instead of Python BytesIO, eliminating per-read bytes allocation in recv_results_metadata and read_type. Modify row_parser.pyx to create a single BytesIOReader from the full remaining buffer and reuse it for both metadata parsing and row parsing, instead of using Python BytesIO for metadata then a separate BytesIOReader for rows. Benchmarks (taskset -c 0, Python 3.14, Cython): recv_results_metadata only (10 simple cols): 6082 -> 1265 ns (4.8x) recv_results_rows (10 cols, 0 rows): 11068 -> 5652 ns (1.96x) recv_results_rows (10 cols, 10 rows): 29590 -> 24759 ns (1.20x) recv_results_rows (10 cols, 100 rows): 181053 -> 173446 ns (1.04x) recv_results_rows (50 cols, 0 rows): 44904 -> 20569 ns (2.18x) Harden read_binary_longstring_br() against a malformed/corrupted frame carrying a negative [bytes] length for paging_state or result_metadata_id. BytesIOReader.read() treats any negative n as "read to end of buffer" instead of raising, and slicing a char* with that same unvalidated (possibly very negative) length is not bounds-checked; on CPython this currently surfaces as an opaque SystemError from PyBytes_FromStringAndSize rather than a clean, catchable error. Reject negative lengths explicitly with a ValueError, mirroring the existing raw_val_size <= 0 check in get_buf() (ioutils.pyx) and the size < 0 check in slice_buffer() (buffer.pxd) used elsewhere in the Cython layer. read_string_br/read_binary_string_br read an unsigned uint16_t length so they cannot go negative; only the signed int32_t path needed this. Make make_recv_results_metadata() take its type objects (type_codes_map, CUSTOM_TYPE, ListType, SetType, MapType, TupleType, UserType, lookup_casstype) as parameters instead of importing them itself from cassandra.protocol/cassandra.cqltypes. cassandra.metadata_parser is imported from inside cython_protocol_handler(), which itself runs while cassandra.protocol's own module body is still executing; a module-level "from cassandra.protocol import ResultMessage, CUSTOM_TYPE" from within metadata_parser is therefore a self-referential import back into a partially-initialized module. It happened to work only because ResultMessage/CUSTOM_TYPE are defined earlier in protocol.py's source than cython_protocol_handler() is invoked -- the same class of import-order fragility that separately broke cassandra.cython_deps.HAVE_CYTHON detection. cython_protocol_handler() already has all of these as module-level names with no import needed, so passing them in removes the dependency on that ordering entirely. Verified this PR's generic wire-level column metadata parsing is unaffected by, and compatible with, the separate schema-parsing changes that trim system_schema.columns to a narrower SELECT and replace dict_factory with _RowView/_row_factory (name-keyed access via result.column_names, which is derived from column_metadata regardless of how many columns the server returns) -- and with the newly landed SCYLLA_USE_METADATA_ID extension, which does not change the recv_results_metadata wire format or flag bits this parser reads. Add tests/unit/cython/test_metadata_parser.py: negative-length rejection (reproduces the SystemError without the fix), NO_METADATA_FLAG handling, and parity between the Cython and pure-Python recv_results_metadata for both the global-tables-spec and per-column wire layouts. Address review feedback: - BytesIOReader gains an explicit seek(new_pos) method, and row_parser.pyx's exception-recovery rewind (reader.pos = rows_start_pos) now goes through it instead of assigning reader.pos directly. Investigated the claim that this rewind was incomplete because BytesIOReader also tracks a `buf_ptr` cursor: read() always computes `buf_ptr + pos`, and `buf_ptr` is set once in __init__ and never reassigned anywhere in the codebase (confirmed via git history back to bytesio.pyx's introduction), so `pos` is the only mutable cursor and there is no actual desync -- the pre-PIN code's "reader.buf_ptr = reader.buf" was a redundant self-assignment, not a necessary reset (it existed because that code built a fresh, row-data-only reader each time, so buf_ptr and pos=0 were simply restating the reader's initial state). seek() still improves on the old direct-field-access pattern by giving callers a single, explicit entry point. Regression test in tests/unit/cython/test_row_parser.py exercises recv_results_rows' exception-recovery path with a ColumnParser that partially consumes the reader before failing, and asserts the re-parse lands on the exact same bytes; tests/unit/cython/test_bytesio.py/bytesio_testhelper.pyx add a direct seek() unit test. - make_recv_results_metadata()/_read_type_br() now take NotSupportedError as an injected parameter, the same way CUSTOM_TYPE/ListType/etc. are already injected, instead of doing "from cassandra.protocol import NotSupportedError" inside _read_type_br's unknown-type-code branch. That runtime import contradicted this PR's own stated rationale for injecting the other type objects (avoiding self-referential imports back into cassandra.protocol while it is still initializing). cython_protocol_handler() now passes its own module-level NotSupportedError through. tests/unit/cython/test_metadata_parser.py's new test_unknown_type_code_raises_injected_exception passes in a stand-in exception class and asserts *that* class is raised, proving the raise goes through the injected closure rather than a hidden import. - _read_type_br()'s UserType branch no longer special-cases num_fields == 0 into empty names/types tuples. Verified empirically that ResultMessage.read_type() (the pure-Python path) raises ValueError for this same input, because zip(*()) unpacked into two variables raises when there are zero fields -- and a real UDT always has at least one field, so num_fields == 0 only ever occurs for malformed/corrupted wire data. Removing the special case lets the Cython path fail the same way instead of silently returning a UDT with no fields, closing the divergence from the pure-Python path for this malformed input. tests/unit/cython/test_metadata_parser.py's new test_zero_field_user_type_matches_pure_python drives both paths with the same zero-field UserType wire bytes and asserts both raise ValueError. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
5854f6a to
908cad0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
tests/unit/cython/test_row_parser.py:94
- This test still uses
_no_op_recv_results_metadata_br, which explicitly consumes no bytes, so the savedrows_start_posremains 0. Valid prefix rows move the cursor before the failure, but they do not exercise restoring the newly important nonzero metadata boundary claimed by the docstring. Use the real metadata parser with a metadata prefix, or a Cython helper that consumes such a prefix, so this regression test actually verifies a nonzero rewind target.
recv_results_rows = make_recv_results_rows(ListParser(), _no_op_recv_results_metadata_br)
cassandra/metadata_parser.pyx:99
- The new recursive collection decoding is not exercised by the added tests: the parity case only uses scalar type
0x0009. A regression in subtype recursion orapply_parameters()for list/set/map would therefore pass CI. Add wire-format parity cases againstResultMessage.read_type()for these collection branches (including nesting).
if typeclass is ListType or typeclass is SetType:
subtype = _read_type_br(reader, type_codes_map, user_type_map,
ListType, SetType, MapType, TupleType, UserType,
CUSTOM_TYPE, lookup_casstype, NotSupportedError)
typeclass = typeclass.apply_parameters((subtype,))
elif typeclass is MapType:
| into `pos` (and, in older code, the always-redundant `buf_ptr`) | ||
| directly at the call site. | ||
| """ | ||
| self.pos = new_pos |
| cdef list column_metadata = [None] * colcount | ||
|
|
||
| if flags & _FLAGS_GLOBAL_TABLES_SPEC: | ||
| ksname = read_string_br(reader) | ||
| cfname = read_string_br(reader) | ||
| for i in range(colcount): | ||
| colname = read_string_br(reader) | ||
| coltype = read_type_br_closure(reader, user_type_map) | ||
| column_metadata[i] = (ksname, cfname, colname, coltype) | ||
| else: | ||
| for i in range(colcount): | ||
| ksname = read_string_br(reader) | ||
| cfname = read_string_br(reader) | ||
| colname = read_string_br(reader) | ||
| coltype = read_type_br_closure(reader, user_type_map) | ||
| column_metadata[i] = (ksname, cfname, colname, coltype) |
Summary
Add
cassandra/metadata_parser.pyxwith zero-copy metadata parsing usingBytesIOReader, eliminating per-readbytesallocation that dominatesrecv_results_metadataandread_typecost in the Cython path.Changes
New file:
cassandra/metadata_parser.pyx— Cythoncdef inlinefunctions (read_int_br,read_short_br,read_string_br,read_type_br) operating onBytesIOReaderinstead of PythonBytesIO. A factory functionmake_recv_results_metadata()returns a closure that captures the type-code map and type objects once.Modified:
cassandra/row_parser.pyx— Creates a singleBytesIOReaderfrom the full remaining buffer and reuses it for both metadata parsing and row parsing (previously: PythonBytesIOfor metadata, then a separateBytesIOReaderfor rows). Error recovery path correctly saves/restores reader position.Modified:
cassandra/protocol.py—cython_protocol_handler()imports the new metadata parser and passes it tomake_recv_results_rows().Benchmarks
All measurements:
taskset -c 0, Python 3.14.3, Cython compiled, origin/master vs this PR.recv_results_metadataonly (isolated)Collections are limited by
apply_parameterscost (addressed separately in #794).recv_results_rowsend-to-end (old Cython vs new Cython)The improvement is entirely in metadata parsing (~2x for 0-row cases). As row count increases, metadata cost becomes a smaller fraction of total time, so the speedup dilutes. The 1000-row case is dominated by row deserialization and shows no change.
Tests
231 passed, 1 skipped — identical to baseline.
Notes
recv_prepared_metadata) is unchanged — it still uses PythonBytesIOinherited fromResultMessage. Prepared statements are parsed once perPREPARE, not on the hot path.setup.pyautomatically picks up the new.pyxfile via the existingcassandra/*.pyxglob.DEFin the.pyxfile (documented with a sync warning comment).