perf: Cache cassandra type parsing with LRU caching (hundreds of ns improvements - x1.1-1.6 speedup) - #690
perf: Cache cassandra type parsing with LRU caching (hundreds of ns improvements - x1.1-1.6 speedup)#690mykaul wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request optimizes custom type parsing by adding LRU caching to frequently-called type lookup functions and implementing a fast path for simple types without parameters. The optimization aims to reduce repeated string manipulation and regex scanning for type lookups that occur frequently during query execution.
Changes:
- Added
@functools.lru_cache(maxsize=256)decorators tolookup_casstype_simpleandparse_casstype_argsfunctions - Implemented fast-path optimization in
lookup_casstypeto avoid regex scanning for simple types (those without parentheses) - Removed error handling wrapper from
lookup_casstype, changing behavior to return UnrecognizedType instead of raising ValueError for invalid types - Added benchmark script demonstrating cache effectiveness for repeated type lookups
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| cassandra/cqltypes.py | Added LRU caching to type parsing functions, removed error handling wrapper, cleaned up unused variable, optimized type lookup with fast path |
| tests/unit/test_types.py | Updated test to reflect new behavior where invalid type names create UnrecognizedType instead of raising ValueError |
| benchmarks/cache_benefit.py | New benchmark script demonstrating LRU cache benefits for repeated type lookups with various type complexities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Cache lookup_casstype_simple() and parse_casstype_args() with @functools.lru_cache() to avoid repeated string manipulation and regex scanning when the same type strings are resolved multiple times (common during schema parsing and query result deserialization). Both caches use an explicit, bounded maxsize (4096) instead of the implicit default: type strings can include server-supplied custom type class names (protocol.py CUSTOM_TYPE) and unrecognized type names, so the cache is capped to bound worst-case memory growth against a misbehaving/malicious server, while remaining generous enough not to thrash for realistic schemas. Also fixes UserType.evict_udt_class() (invoked by Cluster.register_user_type()) to clear the parse_casstype_args() cache. Without this, re-parsing the same UDT type string after an eviction would keep returning the stale, pre-eviction class instead of going through make_udt_class() again, silently defeating the eviction. Also fixes an unused variable warning (prev_names -> _). Includes a pytest-benchmark comparison (cached vs uncached) and unit tests covering cache correctness and the UDT eviction/invalidation interaction.
|
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 `master` (was ~84 commits behind) and pushed a few fixes into the existing commit:
All 10 existing review threads were already resolved before this push and remain so. Full |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
cassandra/cqltypes.py:203
- Caching this lookup makes later type registrations invisible.
CassandraTypeType.__new__can replace an entry in_casstypeswhenever a client defines aCassandraTypesubclass (astest_parse_casstype_argsdoes), but neither this cache norparse_casstype_argsis invalidated. For example, looking up unknownFooType, then definingclass FooType(CassandraType), still returns the cached_UnrecognizedTypeinstead of the newly registered class; cached parameterized strings remain stale as well. Invalidate both caches when the metaclass changes the registries, and add a regression test for this ordering.
@functools.lru_cache(maxsize=_CASSTYPE_CACHE_MAXSIZE)
def lookup_casstype_simple(casstype):
cassandra/cqltypes.py:1026
cache_clear()cannot invalidate a parse already in flight.lru_cacheexecutes a miss outside its internal lock, so a concurrent parse can obtain the old UDT class, this method can evict and clear, and then the first thread can insert that stale class back into the now-empty LRU cache. Subsequent lookups again mask the eviction. Please make invalidation generation-aware or otherwise synchronize parsing and eviction so pre-eviction misses cannot populate the post-eviction cache.
parse_casstype_args.cache_clear()
Summary
Cache
lookup_casstype_simple()andparse_casstype_args()with@functools.lru_cache()to avoid repeated string manipulation and regex scanning when the same type strings are resolved multiple times (common during schema parsing and query result deserialization).The fast-path for simple types (without parentheses) was already merged separately. This PR adds caching on top of that.
Also fixes an unused variable warning (
prev_names→_).Includes a pytest-benchmark comparison (cached vs uncached).
Changes
cassandra/cqltypes.py: Addedimport functools,@functools.lru_cache(maxsize=4096)onlookup_casstype_simple()andparse_casstype_args(), fixed unusedprev_namesvariable, and fixedUserType.evict_udt_class()to also clear theparse_casstype_args()cache (see "Cache correctness" below).benchmarks/test_casstype_cache_benchmark.py: New benchmark file with correctness tests and cached vs uncached performance comparisons.tests/unit/test_types.py: Added unit tests covering cache hit/miss behavior and the UDT eviction/invalidation interaction described below.Benchmark results
lookup_casstype_simple— clear wins from cacheUTF8Type)o.a.c.db.marshal.UTF8Type)parse_casstype_args— modest gains for parameterized typesEnd-to-end
lookup_casstype(mixed simple + parameterized)The biggest gains are on
lookup_casstype_simple, which is called most frequently (every column in every row). Theparse_casstype_argscache helps for parameterized types (maps, lists, sets, tuples) where the regex scanner is the bottleneck.Cache bound (corrected from an earlier revision)
An earlier revision of this PR called
lru_cache()with no arguments and described that as "unbounded... no risk of unbounded memory growth." That claim was wrong on both counts:functools.lru_cache()with no arguments defaults tomaxsize=128(not unbounded), and 128 is small enough that a schema with more than 128 distinct type strings (easy to hit with more than a handful of tables/UDTs) would thrash the cache.Both caches now use an explicit
maxsize=4096. This is deliberately bounded rather thanmaxsize=None/unbounded: type strings reaching these functions aren't always schema-bounded —protocol.py'sCUSTOM_TYPEbranch passes a server-supplied class name straight tolookup_casstype()/lookup_casstype_simple()with no validation, so an unbounded cache would let a misbehaving or malicious server grow the cache without limit. 4096 is generous enough to avoid thrashing on realistic schemas while still capping worst-case memory growth.Cache correctness (UDT invalidation)
UserTypealready has its own per-(keyspace, udt_name)class cache (UserType._cache), with an explicitevict_udt_class()invalidation hook called fromCluster.register_user_type(). Sinceparse_casstype_args()is now also cached, and it's the function that (viaapply_parameters→make_udt_class) produces the class for aUserType(...)type string, the new LRU cache could return a stale, pre-eviction class for a type string that was already cached beforeevict_udt_class()ran — silently masking the eviction. Row (de)serialization itself is unaffected (it goes throughprotocol.py'sread_type(), which callsmake_udt_class()directly and always refreshesmapped_class), but schema-metadata resolution vialookup_casstype()/parse_casstype_args()was not.Fixed by clearing
parse_casstype_args's cache insideevict_udt_class(). Verified with a reproduction (confirmed the bug existed pre-fix, and no longer does), and added a regression test (test_parse_casstype_args_udt_cache_invalidated_on_evict).