perf(cosmos): improve pkrange cache memory usage#46297
Conversation
5afe162 to
fc47cf0
Compare
kushagraThapar
left a comment
There was a problem hiding this comment.
Thanks @tvaron3
I am curious, are there other places where we build the collection routing map? Shall we fix those as well?
6b801a2 to
378f07e
Compare
|
Superseded by shared cache approach. |
378f07e to
342d80a
Compare
1. Share CollectionRoutingMap cache across clients per endpoint. Eliminates N-1 redundant copies when N clients target the same account. 2. Add __slots__ to Range class (64 bytes vs ~250 bytes per instance). 3. Skip .upper() when string is already uppercase. PPCB overhead (150 clients, tracemalloc): Original: 27.4 MB -> Patched: ~0 MB (-100%) At customer scale (200K partitions x 152 clients): ~2.1 GB -> ~14 MB Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
128d459 to
8b03fa2
Compare
…storage Convert raw service response dicts to PKRange namedtuples in both full refresh (_build_routing_map_from_ranges) and incremental update (process_fetched_ranges) paths. PKRange retains only 4 fields (id, minInclusive, maxExclusive, parents) and supports dict-style access for backward compatibility. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Import PKRange in _routing_map_provider_common.py (fixes all emulator tests) - Fix namedtuple name mismatch (_PKRangeBase, not PKRange) for mypy - Use raise-from pattern in PKRange.__getitem__ (pylint W0707) - Move _locks_lock and _collection_locks init into __init__ (pylint W0201) - Add 'pkrange' to cspell dictionary Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
9f82746 to
2cd31c6
Compare
…ge __slots__ Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bd9d741 to
5448e75
Compare
- Widen range_tuples type to List[Tuple[Any, Any]] for PKRange compatibility - Move pkrange word to sdk/cosmos/azure-cosmos/cspell.json (not .vscode) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use upstream's ignoreWords format, add pkrange. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Integration tests (7): - Multi-client shared cache for reads and queries - clear_cache() transparent repopulation and cross-client propagation - Different endpoints isolated - PKRange full CRUD lifecycle and change feed compatibility Fault injection tests (6 sync + 6 async): - 410 Gone triggers cache refresh - Partition split (410/1002) refreshes routing map - Concurrent cache refresh with ThreadPoolExecutor/asyncio.gather - PKRange immutability (namedtuple guarantee) - Transient 503 during PKRange fetch with retry recovery - clear_cache during concurrent reads (no crash/corruption) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run python - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
… parents tuple
Fixes from coding agent harness review iteration 1:
F1: Fix async else branch in refresh_routing_map_provider to use
clear_cache() instead of re-creating SmartRoutingMapProvider
F2: Use dict.clear() in clear_cache() to preserve all client references
(was creating new dict, orphaning other clients' references)
F3: Clear _collection_locks under _locks_lock instead of replacing
F4: Align async clear_cache() with sync (both use .clear())
F5: PKRange.__getitem__ supports integer indexing (int/slice → super())
F6: Convert parents to tuple at construction for true immutability
F8: Fix tests to verify dict identity preserved after clear_cache
F9: Cache .upper() result to avoid double call in slow path
F11: Add changelog entry
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add PKRange.__eq__ for dict comparison (existing tests compare against dicts) - Update partition split retry tests: assert clear_cache() instead of SmartRoutingMapProvider constructor (sync + async) - Fix sync test .close() calls (sync CosmosClient uses context manager, not .close()) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…n, container limits - Fix _session.py: parents.copy() -> list(parents) for tuple compatibility - Add url_connection + tearDown to routing_map_provider tests (cache isolation) - Use existing test containers instead of creating new ones (25-container limit) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Review complete (36:43) Posted 5 inline comment(s). Steps: ✓ context, correctness, cross-sdk, design, history, past-prs, synthesis, test-coverage |
Fixes from the @sdkReviewAgent inline comments on PR Azure#46297 plus the CI test failure introduced by the conftest reset fixture. C1 — TOCTOU on _released (sync + async release()): Move the check-and-set of self._released INSIDE the _shared_cache_lock block. Previously two concurrent callers (e.g. __exit__ racing __del__) could both pass the early-return guard before either set the flag, then both decrement the refcount. Added a threaded barrier-based regression test that demonstrates the fix. C2 — Sync CosmosClient.close(): Added close() to sync CosmosClient mirroring the async client's close(). Now that release() manages process-global refcounts, users that don't use 'with' need a deterministic teardown path. Delegates to __exit__. C3 — Comment correctness: Fixed misleading comment on _shared_cache_lock claiming sync and async modules share state — they don't, each module has its own globals. Also fixed the refcount comment that said clear_cache decrements (it does not — only release() does). C4 — _session.py:386 regression coverage: Added focused unit tests in test_session_token_unit.py for the list(pk_range[0].get('parents') or ()) migration: PKRange-tuple input, None parents, empty parents, tuple parents, and the parents-then-self walk semantics. C5 — release() lifecycle coverage: Added 8 sync + 4 async lifecycle tests in tests/routing/: - construct increments refcount - release decrements / multi-client decrement - release evicts all four globals at zero - release does not evict with other clients alive - release is idempotent (sequential double-call) - concurrent release does not double-decrement (TOCTOU regression) - __del__ fallback releases when client teardown was skipped - clear_cache does not change refcount Test failure fix — test_multi_client_shared_cache_queries: Added _populate_cache helper to the sync integration test that calls PartitionKeyRangeCache.get_routing_map directly (mirroring the async sibling test). The previous version asserted that query_items(... cross_partition=True) populated _collection_routing_map_by_item, which is an implementation detail. The autouse conftest fixture exposed this fragility — the test had been passing only by accident due to cache state left by earlier tests. Teardown completeness: Updated tearDown / tearDownClass in both routing/test_shared_pk_range_cache(_async).py and test_shared_cache_integration(_async).py to clear ALL FOUR shared- cache globals (_shared_routing_map_cache, _shared_collection_locks, _shared_locks_locks, _shared_cache_refcounts) rather than only the routing-map dict. Avoids order-dependent leaks and refcount drift. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- cosmos_client.py: disable specify-parameter-names-in-call on __exit__(None, None, None) — sentinels are positional by Python convention. - routing_range.py: add :param/:returns/:rtype to PKRange.__contains__ docstring. - cspell.json: add 'toctou' to ignoreWords (used in race-condition comments). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run python - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
✅ Review complete (44:14) Posted 2 inline comment(s). Steps: ✓ context, correctness, cross-sdk, design, history, past-prs, synthesis, test-coverage |
…ghputFraction Address xinlian12's two latest review comments on PR Azure#46297, plus retain two non-routing PKR fields based on bluebird-grounded review. clear_cache: async -> sync - aio/routing_map_provider.py: clear_cache no longer async (no awaits inside, uses threading.Lock + dict.clear()). Mirrors the sync release() signature. - aio/_cosmos_client_connection_async.py: drop await from the 2 callers. - tests/test_partition_split_retry_unit_async.py: AsyncMock -> MagicMock. - tests/routing/test_shared_pk_range_cache_async.py, tests/test_shared_cache_fault_injection_async.py, tests/test_shared_cache_integration_async.py: drop await from clear_cache call sites in async tests. PKRange: retain status and throughputFraction - routing_range.py: add status and throughputFraction to _PKRangeBase namedtuple with defaults=(None, None) for back-compat. Add Status and ThroughputFraction constants. - collection_routing_map.py + _routing_map_provider_common.py: propagate both fields when constructing PKRange from raw service dicts (full-load and incremental merge paths). Tests - test_shared_pk_range_cache.py: add test_pkrange_contains_truthy_presence_for_parents covering parents=() (most common production case, partition has never split). - test_shared_pk_range_cache.py: add test_pkrange_status_and_throughput_fraction_fields_roundtrip covering default-None back-compat plus explicit values via dict-style access and __contains__ truthy-presence semantic. All 143 routing/cache/split-retry tests pass locally against a live account. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run python - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
simorenoh
left a comment
There was a problem hiding this comment.
_resolve_endpoint() is identical in sync/async so can be moved to routing_common and the PkRange construction is also duplicated in collection_routing_map and in routing_common - won't block on that though, thanks Tomas!
Per xinlian's review (PR Azure#46297): two duplications were called out: 1. _resolve_endpoint() was identical in sync and async modules. Moved to _routing_map_provider_common.py; both modules import the shared implementation. Prevents silent fallback-shape divergence that would fragment the per-endpoint shared cache. 2. PKRange construction was duplicated in both code paths: - collection_routing_map._build_routing_map_from_ranges (full build) - _routing_map_provider_common.process_fetched_ranges (incremental merge) Added PKRange.from_dict(raw) classmethod factory in routing_range.py; both call sites now use it. Field-mapping policy lives in exactly one place — adding/removing a field touches one line, not two. Net diff: 32 lines deduplicated across 3 files. No behavior change. All 143 existing tests in tests/routing/, tests/test_partition_split_retry_unit*, tests/test_shared_cache_integration*, tests/test_shared_cache_fault_injection_async still pass against tomasvaron-cdb. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run python - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Build Analyze pylint job (build 6228688) flagged C4740(docstring-missing-type) on _resolve_endpoint after it was moved to _routing_map_provider_common.py in eab73eb. The original sync/async versions had `client: Any` and `-> str` annotations plus the matching `:type client: Any` docstring line — those were dropped during the move. Restored the function signature to `def _resolve_endpoint(client: Any) -> str:` (matching the originals) and added the missing `:type client: Any` docstring entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Slims the cached `PartitionKeyRange` from 14 fields to 6, mirroring memory optimization #2 from [Azure SDK for Python PR #46297](Azure/azure-sdk-for-python#46297). The wire contract is unchanged — the dropped JSON keys are silently ignored by serde on deserialization. ## Motivation The routing-map cache holds one `PartitionKeyRange` per `(container, range)` pair. With many containers — or several `CosmosClient` instances against the same account — that footprint dominates the per-container heap. The Cosmos service returns ~14 fields on `/pkranges`, but the routing layer reads only a handful. This PR slims the cached struct from **14 fields (~232 B)** to **6 fields (112 B on 64-bit)**. ## Changes ### `azure_data_cosmos_driver` **`PartitionKeyRange` slimmed:** the cached struct now retains the four fields the routing layer consults (`id`, `min_inclusive`, `max_exclusive`, `status`) plus `throughput_fraction` and `parents`, kept for downstream consumers that read them directly. Dropped: `resource_id`, `self_link`, `etag`, `timestamp`, `rid_prefix`, `target_throughput`, `lsn`, `owned_archival_pk_range_ids`. - `status` is the only retained field beyond the geometry triple consulted on the routing hot path: `validate_and_build_index` reads it to compute `highest_non_offline_pk_range_id` for split detection. - `throughput_fraction` and `parents` are retained on the cached representation for downstream consumers; the routing layer itself does not consult them. **`PartialEq` / `Hash` relaxed** to the routing-relevant identity tuple `(id, min_inclusive, max_exclusive)` — `_rid` is no longer in the comparison key. A grep across the workspace confirmed `PartitionKeyRange` is never used as a `HashMap` / `HashSet` / `BTreeMap` / `BTreeSet` *key* in this crate. ### Tests - `cached_size_stays_small` — asserts `size_of::<PartitionKeyRange>() <= 120 B` (re-bloat tripwire) AND `== 112 B` on 64-bit (silent layout-regression tripwire). - `deserialization_ignores_stripped_metadata_fields` — asserts that a service payload carrying every field still parses, that `throughput_fraction` is read as `0.5`, and that the dropped fields are silently absorbed. - `try_combine_online_to_offline_recomputes_highest_non_offline` — extended with the recovery flip (Online → Offline → Online round-trip) so split-detection semantics are pinned across the field reduction. ## Validation - `cargo fmt --check` clean - `cargo clippy -p azure_data_cosmos_driver --all-features --all-targets -- -D warnings` clean - `cargo test -p azure_data_cosmos_driver --lib --all-features` — **1439 passed** - `cspell` clean ## Reviewer notes The slim struct is a public-API breaking change (8 `pub` fields removed, `PartialEq` / `Hash` semantics relaxed); CHANGELOG documents both in a single entry. There is no behavioral change to routing or to the wire contract. A microbenchmark for the cache hot path was prototyped during development but is **not part of this PR** — it will be submitted separately under the `azure_data_cosmos_benchmarks` crate so reviewers of the model change can focus on the breaking-change surface here. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reduce routing-map memory by dropping fields from each cached PartitionKeyRange that the SDK does not need to keep in heap for the lifetime of the CollectionRoutingMap. The dropped-field set is kept identical to the equivalent Python optimization in Azure/azure-sdk-for-python#46297 rather than being derived from current Java-SDK usage: Dropped: _rid, _etag, ridPrefix, _self, ownedArchivalPKRangeIds, _ts, lsn Kept: id, minInclusive, maxExclusive, parents, throughputFraction, status, _lsn, and any future field the service starts returning (forward-compatible by deny-list, not allow-list). Cross-SDK consistency matters here: a field that Java does not consume today may become useful tomorrow, and aligning with Python's audited deny-list ensures both SDKs make the same memory-vs-future-flexibility trade-off. Using ObjectNode.deepCopy() + remove() rather than rebuilding from getters means unknown future fields automatically flow through with no SDK change. Implementation: in RxPartitionKeyRangeCache#updateRoutingMap, deep-copy the input PartitionKeyRange's propertyBag and remove the 7 dropped fields before wrapping it in a new PartitionKeyRange that is handed to InMemoryCollectionRoutingMap. PartitionKeyRange#equals / #hashCode already key on id/min/max only, so the slim copies are value-equal to the originals. No public-API or wire change. Memory motivation: typical PartitionKeyRange JSON carries large strings (_self path, base64 _rid, quoted _etag GUID/timestamp), saving roughly ~700-900 B per range. Scales as O(clients x collections x pkRanges). Unit tests added in RxPartitionKeyRangeCacheTest: - cachedPartitionKeyRangesAreStrippedOfUnusedFields: all 7 dropped fields gone; throughputFraction / status / _lsn / parents preserved; original instance not mutated. - cachedPartitionKeyRangesPreserveNonEmptyParents: split-merge parents bookkeeping survives the strip. - cachedPartitionKeyRangePassesThroughUnknownFutureField: documents the deny-list (forward-compat) semantics with a hypothetical future field. Full mvn -P unit verify on azure-cosmos-tests: 2530 tests, 0 failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… from ObjectNode Reduce per-instance memory footprint of every PartitionKeyRange the SDK deserializes from a service response by removing fields the SDK does not need to retain in heap. Implementation moved into PartitionKeyRange(ObjectNode) -- the universal constructor funnel used by JsonSerializable#instantiateFromObjectNodeAndType for every FeedResponse<PartitionKeyRange> page (and the change-feed deserialization path). All ingress paths that matter to memory pressure flow through this ctor, so the strip happens once at the boundary and the RxPartitionKeyRangeCache requires no change. Dropped fields are kept identical to the equivalent Python optimization in Azure/azure-sdk-for-python#46297 to keep cross-SDK behaviour aligned: Dropped: _rid, _etag, ridPrefix, _self, ownedArchivalPKRangeIds, _ts, lsn Kept: id, minInclusive, maxExclusive, parents, throughputFraction, status, _lsn, and any future field the service starts returning (deny-list, not allow-list). Cross-SDK consistency matters here: a field that Java does not consume today may become useful tomorrow, and aligning with Python's audited deny-list ensures both SDKs make the same memory-vs-future-flexibility trade-off in one place. The ObjectNode argument is mutated in place rather than deep-copied because every production caller obtains it fresh from Jackson deserialization and does not retain another reference. Tests that need to preserve a fully-populated source object can use ObjectNode#deepCopy before handing it to the constructor. The (String) ctor is intentionally untouched -- it is used only by tests/samples that may want full-fidelity round-trip JSON, and a unit test pins that behaviour. PartitionKeyRange#equals / #hashCode already key on id/min/max only, so slim instances remain value-equal to manually-constructed ones. No public-API or wire change. PartitionKeyRange itself is in the com.azure.cosmos.implementation package. New unit tests in PartitionKeyRangeTest cover: - all 7 deny-listed fields are removed via the ObjectNode ctor - throughputFraction, status, _lsn, parents all preserved - unknown future field passes through (deny-list semantics) - non-empty parents preserved (split-merge bookkeeping) - equals / hashCode / toRange behaviour unchanged - null ObjectNode is tolerated (pre-existing contract) - String ctor retains full fidelity - JsonSerializable.instantiateFromObjectNodeAndType (FeedResponse funnel) routes through the stripped ctor Full mvn -P unit verify on azure-cosmos-tests: 2536 tests, 0 failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the deny-list of 7 known service fields with an allow-list of the 6 fields the SDK actually needs: id, minInclusive, maxExclusive, parents, status, throughputFraction. This mirrors the slots of Python's PKRange namedtuple in Azure/azure-sdk-for-python#46297 and bounds per-instance heap against any future server-side payload growth. Previously the deny-list let new or undocumented fields (e.g. _lsn, or anything the service may add later) silently inflate the cache. Tests updated to pin allow-list semantics, including a test that an unknown future field is dropped rather than passed through. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Four optimizations to drastically reduce
CollectionRoutingMapmemory footprint, motivated by a customer regression where PPCB (Per Partition Circuit Breaker) being silently enabled by default in 4.15.0 caused a ~730 MB heap. EachCosmosClientpreviously kept its own routing-map cache of every partition key range; for accounts with many partitions × many clients this dominates memory.
Changes
1. Shared routing-map cache across clients to the same endpoint
_routing/routing_map_provider.py,_routing/aio/routing_map_provider.pyModule-level
_shared_routing_map_cache: dictkeyed byclient.url_connection. AllPartitionKeyRangeCacheinstances pointing at the same endpoint share a single dict.clear_cache()usesdict.clear()(not reassignment) so every sharing clientcontinues to see the same dict.
Dominant win — independent of per-range size, the cache is now O(endpoints) rather than O(clients × endpoints).
2. Strip unused fields → compact
PKRangenamedtuple_routing/routing_range.py,_routing/routing_map_provider.py,_routing/aio/routing_map_provider.pyThe service returns 13 fields per partition key range;
CollectionRoutingMaponly uses 4 (id,minInclusive,maxExclusive,parents). After fetching, ranges are converted into aPKRangenamedtuple that supports dict-style[key],.get(),in,.items(), and equality with plain dicts for full backward compatibility.Dropped fields:
_rid,_etag,ridPrefix,_self,ownedArchivalPKRangeIds,_ts,lsn.3.
__slots__onRangeand_PartitionHealthInfo_routing/routing_range.py,_global_partition_endpoint_manager_circuit_breaker_core.pyEliminates the per-instance
__dict__(~100 bytes per object). Material in PPCB scenarios with many partitions × many clients.4. Skip redundant
.upper()on hex strings_routing/routing_range.pyRange.__init__previously called.upper()unconditionally onmin/max. Cosmos returns uppercase hex (e.g.10F0F0F0...); we skip the copy when already uppercase.Memory Profiling Results
Test setup (real account, measured with
tracemalloc, retained memory):read_item+ 1upsert_itemAZURE_COSMOS_ENABLE_CIRCUIT_BREAKER=TrueTotal cache memory (MB) — patches 2, 3, 4 measured
PPCB overhead reduction (vs PPCB=false), patches 2-4
Shared-cache impact (#1) — separately measured
The shared cache makes routing-map memory independent of client count for clients sharing an endpoint. Measured with
tracemalloc(real allocated bytes incl. unique string contents) on Python 3.13, 100K PK ranges per cache:Per-cache cost — isolates the per-record container choice (#2-#4 only, no #1):
dictPK-range record +Rangewithout__slots__+ always.upper()PKRangenamedtuple +Range.__slots__+ skip-redundant-.upper()Cluster-wide cost — adds the shared-cache effect (#1):
The shared cache flattens the cost from O(clients × ranges) to O(ranges), which dominates at high client counts. Combined with patches #2-#4, PPCB-enabled customers with many clients per endpoint should see PPCB cache overhead drop close to zero.
Note on a previously-cited number: an earlier draft reported "44 MB → 7 MB / −84%" per 100K entries. That figure used
sys.getsizeof()on bare containers (which excludes string contents and counts the dict's empty-list overhead but not PKRange's tuple overhead) and is not representative of the real allocated footprint. The numbers above usetracemallocpeak-allocation deltas, which is the honest measurement.Test Coverage
New tests added on top of existing routing/cache coverage:
tests/routing/test_shared_pk_range_cache.py(sync) +test_shared_pk_range_cache_async.py(async) — cache sharing, isolation across endpoints,clear_cache()semantics, dict-identity preservation.tests/test_shared_cache_integration.py(sync) +test_shared_cache_integration_async.py(async) — emulator integration covering multi-client cache sharing,clear_cache()repopulation, PKRange compatibility through full CRUD lifecycles, changefeed, and endpoint isolation.
tests/test_shared_cache_fault_injection.py(sync) +test_shared_cache_fault_injection_async.py(async) — fault-injection coverage for the shared cache.tests/test_routing_map.py—PKRangedict-equality,__getitem__,.get,__contains__,.items().tests/routing/test_collection_routing_map.py,test_routing_map_provider.py(+ async),test_routing_map_provider_unit.py(+ async) — substantial expansion of unit coverage for refresh / merge / overlap behavior.All routing, session, query, and circuit-breaker tests pass on the latest pipeline run.
Reproduction Script