(improvement) Optimize Cython deserialization primitives and add VectorType Cython deserializer (substantial - 11x-30x speedup mainly via DesVectorType.deserialize_bytes with Cython) - #732
Conversation
34dd41e to
9b7697c
Compare
mykaul
changed the title
mykaul
changed the title
9b7697c to
cb2c0c2
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 1. 2. 3. Circular import spuriously breaking Also updated Testing: rebuilt the Cython extensions locally and ran the full unit suite: No review threads existed on this PR to resolve. Still a draft as intended. |
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 improves Cython-based deserialization performance and adds a dedicated Cython deserializer for VectorType, targeting large speedups for numeric vector decoding.
Changes:
- Replace varint hex-string conversion with
int.from_bytes(..., signed=True)in both Python and Cython marshal paths. - Introduce
DesVectorTypewith C-level fast paths (plus optional NumPy path) and updatefind_deserializer()routing for vectors. - Tighten buffer handling by replacing
slice_buffer()withfrom_ptr_and_size()and adding explicit bounds validation in several deserializers.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_types.py | Adds unit tests covering DesVectorType selection and vector subtype routing/fallback behavior. |
| cassandra/marshal.py | Optimizes Python varint_unpack() using int.from_bytes. |
| cassandra/ioutils.pyx | Optimizes read_int() using memcpy + ntohl for alignment-safe decoding. |
| cassandra/deserializers.pyx | Adds DesVectorType, NumPy fast-path, subtype-aware routing, and additional bounds checks / buffer slicing changes. |
| cassandra/cython_marshal.pyx | Optimizes numeric unpacking using ntohs/ntohl + int.from_bytes and adds a float-specific swap path. |
| cassandra/buffer.pxd | Removes slice_buffer() and documents/implements from_ptr_and_size() including negative-size sentinels. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…y large vector deserialization Add test_vector_cython_deserializer_variable_size_subtype to verify find_deserializer()'s current dispatch for a VectorType with a variable-size subtype (e.g. UTF8Type). cassandra.deserializers has no dedicated Des* class for VectorType today, and VectorType is not a subclass of any of the collection types find_deserializer() special-cases, so it always falls through to GenericDeserializer -- regardless of whether the subtype has a fixed serialized size or not. GenericDeserializer simply delegates to the pure Python VectorType.deserialize(), which correctly round-trips variable-size subtypes; the test asserts both the dispatch and the round-trip. A Cython fast-path deserializer for VectorType (DesVectorType), with dispatch that fast-paths fixed-size subtypes while leaving variable-size subtypes on GenericDeserializer, is being developed in companion PRs (scylladb#689, scylladb#732). Neither has merged, so DesVectorType does not exist on this branch or on master; this test intentionally does not depend on it. Once one of those PRs lands, this test should be revisited to also assert the Cython dispatch/behavior for fixed-size subtypes. Add test_vector_numpy_large_deserialization to exercise VectorType deserialization for vectors with >= 32 elements across all supported fixed-size numeric types (float, double, int32, int64). VectorType has no numpy-specific fast path today; the test name/threshold is forward-looking and documents that, guarding correctness of the existing implementation at representative embedding sizes in the meantime. Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>
…y large vector deserialization Add test_vector_cython_deserializer_variable_size_subtype to verify find_deserializer()'s current dispatch for a VectorType with a variable-size subtype (e.g. UTF8Type). cassandra.deserializers has no dedicated Des* class for VectorType today, and VectorType is not a subclass of any of the collection types find_deserializer() special-cases, so it always falls through to GenericDeserializer -- regardless of whether the subtype has a fixed serialized size or not. GenericDeserializer simply delegates to the pure Python VectorType.deserialize(), which correctly round-trips variable-size subtypes; the test asserts both the dispatch and the round-trip. A Cython fast-path deserializer for VectorType (DesVectorType), with dispatch that fast-paths fixed-size subtypes while leaving variable-size subtypes on GenericDeserializer, is being developed in companion PRs (scylladb#689, scylladb#732). Neither has merged, so DesVectorType does not exist on this branch or on master; this test intentionally does not depend on it. Once one of those PRs lands, this test should be revisited to also assert the Cython dispatch/behavior for fixed-size subtypes. Add test_vector_numpy_large_deserialization to exercise VectorType deserialization for vectors with >= 32 elements across all supported fixed-size numeric types (float, double, int32, int64). VectorType has no numpy-specific fast path today; the test name/threshold is forward-looking and documents that, guarding correctness of the existing implementation at representative embedding sizes in the meantime. Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>
…y large vector deserialization Add test_vector_cython_deserializer_variable_size_subtype to verify find_deserializer()'s current dispatch for a VectorType with a variable-size subtype (e.g. UTF8Type). cassandra.deserializers has no dedicated Des* class for VectorType today, and VectorType is not a subclass of any of the collection types find_deserializer() special-cases, so it always falls through to GenericDeserializer -- regardless of whether the subtype has a fixed serialized size or not. GenericDeserializer simply delegates to the pure Python VectorType.deserialize(), which correctly round-trips variable-size subtypes; the test asserts both the dispatch and the round-trip. A Cython fast-path deserializer for VectorType (DesVectorType), with dispatch that fast-paths fixed-size subtypes while leaving variable-size subtypes on GenericDeserializer, is being developed in companion PRs (scylladb#689, scylladb#732). Neither has merged, so DesVectorType does not exist on this branch or on master; this test intentionally does not depend on it. Once one of those PRs lands, this test should be revisited to also assert the Cython dispatch/behavior for fixed-size subtypes. Add test_vector_numpy_large_deserialization to exercise VectorType deserialization for vectors with >= 32 elements across all supported fixed-size numeric types (float, double, int32, int64). VectorType has no numpy-specific fast path today; the test name/threshold is forward-looking and documents that, guarding correctness of the existing implementation at representative embedding sizes in the meantime. Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>
….from_bytes Performance improvements to serialization/deserialization hot paths: 1. unpack_num(): Use ntohs()/ntohl() for 16-bit and 32-bit integer types instead of byte-by-byte swapping loop. These compile to single bswap instructions on x86, providing more predictable performance. 2. read_int(): Simplify to use ntohl() directly instead of going through unpack_num() with a temporary Buffer. 3. varint_unpack(): Replace hex string conversion with int.from_bytes(). This eliminates string allocations and provides 4-18x speedup for the function itself (larger gains for longer varints). 4. Remove slice_buffer() and replaced with direct assignment 5. _unpack_len() is now implemented similar to read_int() Also removes unused 'start' and 'end' variables from unpack_num(). End-to-end benchmark shows ~4-5% improvement in row throughput. Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>
…helpers Add buffer bounds validation to Cython deserializers for safety against malformed buffers, refactor to use from_ptr_and_size() helper consistently, and add float ntohl() specialization for consistency with int32/int16 paths. Changes: - subelem(): Add CQL protocol-compliant value handling (NULL/-1, not-set/-2, invalid/<-2) with bounds checking - _unpack_len(): Add bounds check and use memcpy for alignment safety - DesTupleType: Add defensive bounds checking for tuple item lengths - DesCompositeType: Add bounds validation for composite element lengths - Refactor 4 locations to use from_ptr_and_size() instead of manual Buffer field assignment - Add float branch to unpack_num(): reinterpret bits as uint32, ntohl(), reinterpret back (consistent with int16/int32 intrinsic paths) - Add from_ptr_and_size() declaration to buffer.pxd Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>
…izer Addded DesVectorType Cython deserializer with C-level optimizations for improved performance in row parsing for vectors. The deserializer uses: - Direct C byte swapping (ntohl) for numeric types - Memory operations without Python object overhead - Unified numpy path for large vectors (≥32 elements) - Hand-written fast paths for small vectors (<32 elements) Only float/double/int32/bigint subtypes get the fast C-level path: these are the only VectorType subtypes that are genuinely fixed-width on the wire (they have a real serial_size() override). Every other subtype -- including variable-length/vint-prefixed ones like text, varint, and smallint (ShortType has no serial_size() override and is vint-prefixed per element in a vector, not a raw 2-byte value) -- falls back to GenericDeserializer, which delegates to the pure-Python VectorType.deserialize() and handles them correctly. find_deserializer() is subtype-aware for VectorType so it never routes an unsupported subtype to the fast path, where it would otherwise surface as an uncaught ValueError during row parsing. Also avoid importing cassandra.cython_deps from this module: cython_deps imports cassandra.row_parser, which imports this module for make_deserializers(), so importing cython_deps back from here closes an import cycle. Depending on import order this makes Python see cython_deps as partially initialized while resolving HAVE_NUMPY, raising ImportError, which cython_deps' own try/except silently turns into HAVE_CYTHON = False even though the Cython extensions built and imported fine. Do our own independent numpy-availability check instead. Performance improvements: - Small vectors (3-4 elements): 4.4-4.7x faster - Medium vectors (128 elements): 1.0-1.5x faster - Large vectors (384-1536 elements): 0.9-1.0x (marginal) (measured for the fixed-width fast path subtypes) The Cython deserializer is automatically used by the row parser when available via find_deserializer(). Includes unit tests and benchmark code. Follow-up commits will try to get Numpy arrays, and perhaps more. Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>
The 'values' list was allocated but never used — the method builds results directly into a pre-allocated tuple via tuple_set(res, i, item). Removes one unnecessary list allocation per tuple deserialization.
cb2c0c2 to
0974137
Compare
Summary
Optimize foundational Cython byte-unpacking and add a dedicated VectorType Cython deserializer.
Commits (4, squashed from 7)
1. Optimize Cython byte unpacking with ntohs/ntohl and int.from_bytes
unpack_num()withntohs()/ntohl()intrinsics for 16/32-bit types (compiles to singlebswapon x86)varint_unpack()hex-string-based conversion withint.from_bytes(term, 'big', signed=True)— 7.7x speedupread_int()to direct pointer cast +ntohl()slice_buffer(), replace all call sites withfrom_ptr_and_size()#ifdef _WIN32forwinsock2.hvsarpa/inet.h2. Optimize float deserialization with ntohl() intrinsic
uint32_t, applyntohl(), reinterpret back to floatfrom_ptr_and_size()helper consistentlysubelem(), bounds checks in_unpack_len(),DesTupleType,DesCompositeType)3. Optimize VectorType deserialization with Cython deserializer
DesVectorTypeclass with specialized deserialization methods:_deserialize_float(): C-levelmemcpy+ntohl+ pointer-cast (no Python dispatch per element)_deserialize_double()/_deserialize_int64(): 8-byte manual byte-swap_deserialize_int32():memcpy+ntohl+ cast_deserialize_int16():ntohscastfind_deserializer()for the Cython row parser4. Remove dead
values = []in DesTupleType.deserializevalueslist was allocated but never used — results built directly into pre-allocated tuple viatuple_set()Benchmark Results
All benchmarks:
min(timeit.repeat(number=N, repeat=5)), per-call nanoseconds.Machine: idle Linux workstation, Cython extensions compiled.
Primitives (via
CqlType.deserialize())The ntohs/ntohl change replaces a byte-swap loop that was already fast for 2/4-byte types. The big win is
varint_unpack()whereint.from_bytes()replaces hex-string conversion.VectorType — Python path (via
VectorType.deserialize())VectorType — Cython path (via
DesVectorType.deserialize_bytes())The Cython
DesVectorTypeis used by the Cython row parser (find_deserializer()), bypassing the PythonVectorType.deserialize()entirely:Unit tests
640 passed, 49 skipped (baseline: 645 passed, 43 skipped on master).