(improvement) (cython only) cache deserializer instances in find_deserializer and m…#741
Draft
mykaul wants to merge 2 commits intoscylladb:masterfrom
Draft
(improvement) (cython only) cache deserializer instances in find_deserializer and m…#741mykaul wants to merge 2 commits intoscylladb:masterfrom
mykaul wants to merge 2 commits intoscylladb:masterfrom
Conversation
mykaul
changed the title
There was a problem hiding this comment.
Pull request overview
This PR improves performance in the Cython fast-path by caching deserializer lookups and per-column deserializer arrays, avoiding repeated class-resolution work and repeated Deserializer object creation across result sets.
Changes:
- Add module-level Cython dict caches for
find_deserializer()andmake_deserializers()keyed bycqltypeobjects / tuples ofcqltypeobjects. - Update
find_deserializer()/make_deserializers()to consult and populate these caches. - Add a pytest-based benchmark module to compare cached vs uncached behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| cassandra/deserializers.pyx | Adds caching for deserializer instance resolution and for per-column deserializer arrays. |
| benchmarks/test_deserializer_cache_benchmark.py | Introduces correctness checks + pytest-benchmark-style benchmarks for the new caching behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+443
to
+447
| # Cache make_deserializers results keyed on the tuple of cqltype objects. | ||
| # Using the cqltype objects themselves (rather than id()) as keys ensures | ||
| # the dict holds strong references, preventing GC and id() reuse issues | ||
| # with non-singleton parameterized types. | ||
| cdef dict _make_deserializers_cache = {} |
Comment on lines
+471
to
+474
| try: | ||
| return <Deserializer>_deserializer_cache[cqltype] | ||
| except KeyError: | ||
| pass |
Comment on lines
+451
to
+458
| cdef tuple key = tuple(cqltypes) | ||
| try: | ||
| return _make_deserializers_cache[key] | ||
| except KeyError: | ||
| pass | ||
| result = obj_array([find_deserializer(ct) for ct in cqltypes]) | ||
| _make_deserializers_cache[key] = result | ||
| return result |
Comment on lines
+21
to
+27
| import pytest | ||
|
|
||
| from cassandra import cqltypes | ||
| from cassandra.deserializers import ( | ||
| find_deserializer, | ||
| make_deserializers, | ||
| ) |
Comment on lines
+23
to
+27
| from cassandra import cqltypes | ||
| from cassandra.deserializers import ( | ||
| find_deserializer, | ||
| make_deserializers, | ||
| ) |
…ake_deserializers Cache find_deserializer() and make_deserializers() results in Cython cdef dict caches keyed on cqltype objects to avoid repeated class lookups and Deserializer object creation on every result set. Using cqltype objects (not id()) as cache keys holds strong references, preventing GC/id-reuse correctness issues with parameterized types. ## Motivation On every result set, make_deserializers(coltypes) is called from row_parser.pyx:37, which in turn calls find_deserializer() for each column type. These functions perform class name lookups and issubclass() chains, then create fresh Deserializer objects -- all redundant work when the same column types appear repeatedly (which is always the case for prepared statements). ## Benchmark results Benchmarks compare the original code (Before) against the new cached implementation (After). find_deserializer (single type lookup): | Variant | Min | Mean | Median | Ops/sec | |---|---|---|---|---| | Before (original) | 266.0 ns | 305.0 ns | 292.0 ns | 3.3 Mops/s | | After (with cache) | 44.0 ns | 49.0 ns | 47.8 ns | 20.4 Mops/s | make_deserializers (5 types): | Variant | Min | Mean | Median | Ops/sec | |---|---|---|---|---| | Before (original) | 1,976 ns | 2,438 ns | 2,435 ns | 410 Kops/s | | After (with cache) | 74.9 ns | 83.5 ns | 81.7 ns | 12,000 Kops/s | make_deserializers (10 types): | Variant | Min | Mean | Median | Ops/sec | |---|---|---|---|---| | Before (original) | 3,553 ns | 3,812 ns | 3,761 ns | 262 Kops/s | | After (with cache) | 89.7 ns | 105.1 ns | 97.6 ns | 9,511 Kops/s | ## Design notes - Caches are cdef dict (C-level, not accessible from Python) for minimal overhead - Cache keys are the cqltype objects themselves, not id(cqltype) -- holds strong references preventing GC and id() reuse - For prepared statements (the hot path), cache hit rate is effectively 100% - Cache is naturally bounded by the number of distinct cqltype objects in use ## Tests All existing unit tests pass (108 passed, 1 skipped).
…ports - Add 256-entry size cap to both _deserializer_cache and _make_deserializers_cache to prevent unbounded growth from non-interned parameterized types in unprepared queries. - Add clear_deserializer_caches() public API so that runtime Des* class overrides (e.g. DesBytesType = DesBytesTypeByteArray for cqlsh) can flush stale cached instances. - Add get_deserializer_cache_sizes() diagnostic helper. - Document override/cache interaction in code comments. - Fix benchmark copyright (DataStax -> ScyllaDB), add pytest.importorskip guards for pytest-benchmark and Cython. - Add 11 unit tests for cache hit/miss, clear, eviction bounds, and size reporting.
mykaul
force-pushed
the
perf/cache-deserializer-lookup
branch
from
5894e22 to
44cdd0d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ake_deserializers
Cache find_deserializer() and make_deserializers() results in Cython cdef dict caches keyed on cqltype objects to avoid repeated class lookups and Deserializer object creation on every result set.
Using cqltype objects (not id()) as cache keys holds strong references, preventing GC/id-reuse correctness issues with parameterized types.
Motivation
On every result set, make_deserializers(coltypes) is called from row_parser.pyx:37, which in turn calls find_deserializer() for each column type. These functions perform class name lookups and issubclass() chains, then create fresh Deserializer objects -- all redundant work when the same column types appear repeatedly (which is always the case for prepared statements).
Benchmark results
Benchmarks compare the original code (Before) against the new cached implementation (After).
find_deserializer (single type lookup):
make_deserializers (5 types):
make_deserializers (10 types):
Design notes
Tests
All existing unit tests pass (108 passed, 1 skipped).
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.