perf(cache): opt-in quick_cache backend for contention-free reads#7953
perf(cache): opt-in quick_cache backend for contention-free reads#7953LuQQiu wants to merge 4 commits into
Conversation
moka records every cache hit into one global read-op channel and lets readers trigger inline housekeeping; at high concurrent hit rates (hundreds of thousands of reads/sec) those shared-cache-line atomics collapse read throughput (measured: negative scaling from 2.9 to 0.3 Mops/s between 1 and 256 readers on a 320-core host). Add a CacheBackend backed by quick_cache, whose S3-FIFO policy records a hit with one reference bit on the entry itself: reads stay 72-152 ns/op flat through 256 readers. LANCE_CACHE_BACKEND=quick opts in; moka remains the default. On a warm 100M-doc fts workload the swap lifts c128 from 181 to 1341 qps (7.4x) with identical results.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an experimental weighted ChangesQuick cache backend
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant LanceCache
participant QuickCacheBackend
participant quick_cache
participant Loader
Caller->>LanceCache: construct with capacity
LanceCache->>QuickCacheBackend: select when LANCE_CACHE_BACKEND=quick
Caller->>QuickCacheBackend: get_or_insert(key)
QuickCacheBackend->>quick_cache: get_value_or_guard_async(key)
quick_cache->>Loader: load on cache miss
Loader-->>QuickCacheBackend: CacheEntry and size_bytes
QuickCacheBackend-->>Caller: entry and hit/miss flag
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
rust/lance-core/src/cache/quick.rs-35-57 (1)
35-57: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winAdd missing doc comments and make the estimate capacity-aware.
QuickCacheBackendandQuickCacheBackend::with_capacityare public API but lack doc comments. Also,estimated_items_capacityshould roughly matchweight_capacity / average item weight; deriving it fromcapacitywill size the cache for small vs. largecapacityvalues instead of always assuming ~1M average-size entries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-core/src/cache/quick.rs` around lines 35 - 57, The public QuickCacheBackend type and its with_capacity constructor need documentation, and the fixed estimated_items value must become capacity-aware. Add concise doc comments to QuickCacheBackend and QuickCacheBackend::with_capacity, then derive the item estimate from capacity and the expected average entry weight before passing it to Cache::with_weighter, preserving the existing cache initialization behavior.
🧹 Nitpick comments (4)
rust/lance-core/src/cache/quick.rs (3)
162-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for
invalidate_prefix.The test covers roundtrip, singleflight dedup, weighted eviction, and clear, but not
invalidate_prefix, which is a newCacheBackendoperation implemented in this file.As per coding guidelines: "Every bugfix and feature must have corresponding tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-core/src/cache/quick.rs` around lines 162 - 214, Add coverage for the new invalidate_prefix operation in test_quick_backend_roundtrip_singleflight_and_eviction: insert multiple entries sharing a prefix plus at least one non-matching entry, invoke invalidate_prefix through the cache API, and assert matching entries are removed while the non-matching entry remains accessible.Source: Coding guidelines
29-33: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
EntryWeighteromits key footprint, unlikeMokaCacheBackend's weigher.
MokaCacheBackend::with_capacityweighskey_footprint(key).saturating_add(entry.size_bytes), but this weigher only countsvalue.size_bytes. Sincesize_bytes()/approx_size_bytes()are documented as "Total weighted size in bytes of all stored entries" / used forDeepSizeOfmemory reporting, switching backends changes what that number actually represents — undermining the stated goal of comparing hit-rate/memory behavior between the two backends.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-core/src/cache/quick.rs` around lines 29 - 33, Update EntryWeighter::weight to include the InternalCacheKey footprint using the same key_footprint calculation as MokaCacheBackend, then add the QuickEntry size with saturating arithmetic while preserving the minimum weight behavior for zero-sized entries. Ensure quick-cache sizing and reporting represent both key and value memory consistently across backends.
95-105: 🚀 Performance & Scalability | 🔵 TrivialFull-cache scan on every
invalidate_prefixcall.This iterates and collects every key in the cache, then removes matches one by one — O(n) in total cache size per call, regardless of how many entries actually match. This mirrors the design limits of quick_cache (no native prefix index), so may be an accepted tradeoff, but worth being aware of for caches with many entries under frequent invalidation (e.g. per-fragment cache invalidation on write-heavy workloads).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-core/src/cache/quick.rs` around lines 95 - 105, Acknowledge the full-cache scan in invalidate_prefix as an inherent quick_cache limitation and assess whether this path is acceptable for frequent prefix invalidation; if optimization is required, replace the per-call full iteration with a suitable prefix-aware indexing or invalidation strategy while preserving removal of every matching InternalCacheKey.rust/lance-core/Cargo.toml (1)
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGate
quick_cachebehind a Cargo feature flag.
quick_cacheis explicitly experimental/opt-in (enabled only viaLANCE_CACHE_BACKEND=quick), but it's added as an unconditional dependency. As per coding guidelines,rust/**/Cargo.toml: "Gate optional or domain-specific dependencies behind Cargo feature flags and prefer separate crates for domain functionality."♻️ Suggested approach
-quick_cache = "0.6" +quick_cache = { version = "0.6", optional = true }Then gate
mod quick;/pub use quick::QuickCacheBackend;inmod.rsand theLANCE_CACHE_BACKENDbranch behind#[cfg(feature = "quick-cache")], and add the feature to[features].Also worth double-checking: the line above (
moka.workspace = true) uses workspace-managed dependency versioning; consider whetherquick_cacheshould be declared in the workspace[workspace.dependencies]for consistency (thoughnum_cpusright below shows this isn't uniformly enforced in this file).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-core/Cargo.toml` at line 31, Gate the quick_cache dependency behind a new quick-cache feature in rust/lance-core/Cargo.toml, preserving workspace dependency conventions where applicable. Update the quick module exports in mod.rs and the LANCE_CACHE_BACKEND quick branch to compile only with #[cfg(feature = "quick-cache")], while keeping the existing behavior for other cache backends unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Other comments:
In `@rust/lance-core/src/cache/quick.rs`:
- Around line 35-57: The public QuickCacheBackend type and its with_capacity
constructor need documentation, and the fixed estimated_items value must become
capacity-aware. Add concise doc comments to QuickCacheBackend and
QuickCacheBackend::with_capacity, then derive the item estimate from capacity
and the expected average entry weight before passing it to Cache::with_weighter,
preserving the existing cache initialization behavior.
---
Nitpick comments:
In `@rust/lance-core/Cargo.toml`:
- Line 31: Gate the quick_cache dependency behind a new quick-cache feature in
rust/lance-core/Cargo.toml, preserving workspace dependency conventions where
applicable. Update the quick module exports in mod.rs and the
LANCE_CACHE_BACKEND quick branch to compile only with #[cfg(feature =
"quick-cache")], while keeping the existing behavior for other cache backends
unchanged.
In `@rust/lance-core/src/cache/quick.rs`:
- Around line 162-214: Add coverage for the new invalidate_prefix operation in
test_quick_backend_roundtrip_singleflight_and_eviction: insert multiple entries
sharing a prefix plus at least one non-matching entry, invoke invalidate_prefix
through the cache API, and assert matching entries are removed while the
non-matching entry remains accessible.
- Around line 29-33: Update EntryWeighter::weight to include the
InternalCacheKey footprint using the same key_footprint calculation as
MokaCacheBackend, then add the QuickEntry size with saturating arithmetic while
preserving the minimum weight behavior for zero-sized entries. Ensure
quick-cache sizing and reporting represent both key and value memory
consistently across backends.
- Around line 95-105: Acknowledge the full-cache scan in invalidate_prefix as an
inherent quick_cache limitation and assess whether this path is acceptable for
frequent prefix invalidation; if optimization is required, replace the per-call
full iteration with a suitable prefix-aware indexing or invalidation strategy
while preserving removal of every matching InternalCacheKey.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 8adc8a79-7d19-48d7-9a5b-0f95643f2b06
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
rust/lance-core/Cargo.tomlrust/lance-core/src/cache/mod.rsrust/lance-core/src/cache/quick.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1a96270 to
1447211
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/lance-core/src/cache/quick.rs (2)
208-219: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnsure the eviction test proves admission.
If every
fill-*insertion is rejected, the existing entries still satisfysize_bytes <= CAPACITYandsize() < 258, so this test passes without caching any fill item. Assert that at least one fill entry was admitted before checking eviction bounds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-core/src/cache/quick.rs` around lines 208 - 219, Update the fill loop in the eviction test to track whether any insert_with_key call for a fill-* key was admitted, and assert that at least one fill entry was cached before the existing size_bytes and size eviction-bound assertions. Preserve the current overfill scenario and bounds checks.
193-206: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the single-flight test concurrent.
The loop awaits each call before starting the next, so the second call is guaranteed to be a cache hit. Start multiple tasks simultaneously against an uncached key, block the loader with a barrier/notification, and assert
loads == 1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-core/src/cache/quick.rs` around lines 193 - 206, The single-flight test currently runs requests sequentially, so it does not verify concurrent loading. Update the test around get_or_insert_with_key to spawn multiple tasks using the same uncached key, synchronize their start, and block the loader with a barrier or notification until all callers are active; then release it, await all results, and assert loads remains 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance-core/src/cache/quick.rs`:
- Around line 57-58: Document the public QuickCacheBackend::with_capacity
constructor with Rustdoc covering byte-weighted capacity, shard-local admission
limits, and a usage example linking to the relevant QuickCacheBackend types or
methods. Keep the implementation unchanged.
---
Outside diff comments:
In `@rust/lance-core/src/cache/quick.rs`:
- Around line 208-219: Update the fill loop in the eviction test to track
whether any insert_with_key call for a fill-* key was admitted, and assert that
at least one fill entry was cached before the existing size_bytes and size
eviction-bound assertions. Preserve the current overfill scenario and bounds
checks.
- Around line 193-206: The single-flight test currently runs requests
sequentially, so it does not verify concurrent loading. Update the test around
get_or_insert_with_key to spawn multiple tasks using the same uncached key,
synchronize their start, and block the loader with a barrier or notification
until all callers are active; then release it, await all results, and assert
loads remains 1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 5b725044-3b5e-4049-9f4a-13cef76d519b
📒 Files selected for processing (1)
rust/lance-core/src/cache/quick.rs
| pub fn with_capacity(capacity: usize) -> Self { | ||
| let estimated_items = (capacity / (TARGET_SHARD_SHARE / 32)).max(1); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Document the public constructor.
QuickCacheBackend::with_capacity is public but lacks Rustdoc describing byte-weighted capacity, shard-local admission limits, and usage examples.
As per coding guidelines, **/*.{rs,md,rst} must “Document all public APIs with examples and links to relevant structs and methods.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance-core/src/cache/quick.rs` around lines 57 - 58, Document the public
QuickCacheBackend::with_capacity constructor with Rustdoc covering byte-weighted
capacity, shard-local admission limits, and a usage example linking to the
relevant QuickCacheBackend types or methods. Keep the implementation unchanged.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
rust/lance-core/src/cache/quick.rs-57-58 (1)
57-58: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the shard-rounding comment.
quick_cacherounds the configured shard count to the next power of two, not “UP to one.” Replace this wording so future maintainers don’t misinterpret the pre-rounding logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-core/src/cache/quick.rs` around lines 57 - 58, Update the shard-rounding comment near the quick_cache configuration to state that quick_cache rounds the configured shard count up to the next power of two, replacing the incorrect “up to one” wording without changing the implementation.Sources: Coding guidelines, MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance-core/src/cache/quick.rs`:
- Around line 69-72: Replace the unconditional 1,000,000 baseline in the cache
initialization logic with an estimate derived from the configured weight budget,
using a bounded explicit average-entry weight and preserving the shard minimum
heuristic. Add regression coverage for small byte-capacity caches to verify the
estimate does not imply excessive resident entries or ghost-key metadata.
---
Other comments:
In `@rust/lance-core/src/cache/quick.rs`:
- Around line 57-58: Update the shard-rounding comment near the quick_cache
configuration to state that quick_cache rounds the configured shard count up to
the next power of two, replacing the incorrect “up to one” wording without
changing the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 09c21bf1-1d2d-4381-9a94-7491b8792295
📒 Files selected for processing (1)
rust/lance-core/src/cache/quick.rs
quick_cache splits its weight budget evenly across shards with no cross-shard borrowing, and silently refuses entries heavier than a shard's share. Declare estimated items as capacity / (share / 32) so the library's own shard sizing keeps every shard's share at or above TARGET_SHARD_SHARE (2 GiB): large caches keep many shards for read concurrency, small caches trade shards for admissible entry size.
1447211 to
b0563fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance-core/src/cache/quick.rs`:
- Line 78: Make QuickCacheBackend::with_capacity propagate the Result from
OptionsBuilder::build() instead of panicking with expect. Update
LanceCache::with_capacity and its callers as needed to propagate or handle
initialization errors while preserving the public cache construction behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: e99002a1-7b33-4ae4-ba16-4ccbc8173d70
📒 Files selected for processing (1)
rust/lance-core/src/cache/quick.rs
| .weight_capacity(capacity as u64) | ||
| .shards(shards) | ||
| .build() | ||
| .expect("quick_cache options"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
curl -fsSL 'https://docs.rs/quick_cache/0.6.0/src/quick_cache/options.rs.html' |
rg -n 'pub fn build|weight_capacity is not set|estimated_items_capacity is not set'Repository: lance-format/lance
Length of output: 415
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '^rust/lance-core/src/cache/quick\.rs$' || true
echo "== file outline =="
ast-grep outline rust/lance-core/src/cache/quick.rs || true
echo "== relevant lines =="
cat -n rust/lance-core/src/cache/quick.rs | sed -n '1,140p'
echo "== dependency/version references =="
rg -n 'quick_cache|OptionsBuilder|build\(\)' rust/Cargo.toml Cargo.lock rust/lance-core -g '*.toml' -g '*.rs'Repository: lance-format/lance
Length of output: 9469
🌐 Web query:
quick_cache 0.6.0 OptionsBuilder build returns Result missing options source
💡 Result:
In the quick_cache crate (version 0.6.0), the build method on OptionsBuilder returns a Result<Options, Error> because it enforces that specific mandatory fields are set by the user [1]. The implementation of the build method explicitly checks for the following required fields, returning an Error if they are missing [1]: 1. weight_capacity: The builder verifies that this has been provided; otherwise, it returns an error stating "weight_capacity is not set" [1]. 2. estimated_items_capacity: The builder verifies that this has been provided; otherwise, it returns an error stating "estimated_items_capacity is not set" [1]. If both of these fields are configured using their respective methods on the OptionsBuilder, the build call will successfully return an Ok(Options) containing the built configuration [1]. Other fields, such as shards, hot_allocation, and ghost_allocation, are optional and will default to predefined values if not explicitly set [1]. Consequently, users typically call.unwrap on the result of.build in examples when they have ensured these required parameters are provided [1][2][3].
Citations:
- 1: https://docs.rs/quick_cache/latest/src/quick_cache/options.rs.html
- 2: https://docs.rs/quick_cache/latest/quick_cache/sync/struct.Cache.html
- 3: https://docs.rs/quick_cache/latest/src/quick_cache/sync.rs.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cache backend selection references =="
cat -n rust/lance-core/src/cache/mod.rs | sed -n '160,210p'
echo "== all QuickCacheBackend call sites =="
rg -n 'QuickCacheBackend::with_capacity|with_capacity\(' rust/lance-core/src/cache mod.rs -g '*.rs'
echo "== Result alias and error constructors near top =="
cat -n rust/lance-core/src/lib.rs | sed -n '1,80p'Repository: lance-format/lance
Length of output: 4136
Make quick cache initialization fallible.
OptionsBuilder::build() returns a Result, but QuickCacheBackend::with_capacity() converts a valid failure to a panic with expect("quick_cache options"). Since LanceCache::with_capacity() also uses this path, changing the constructor would also require changing the public cache constructor or propagating errors explicitly through that boundary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance-core/src/cache/quick.rs` at line 78, Make
QuickCacheBackend::with_capacity propagate the Result from
OptionsBuilder::build() instead of panicking with expect. Update
LanceCache::with_capacity and its callers as needed to propagate or handle
initialization errors while preserving the public cache construction behavior.
Source: Coding guidelines
Matches the moka backend's accounting (key footprint + entry bytes) so the two backends have the same capacity semantics.
a79504b to
c6a63e9
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rust/lance-core/src/cache/quick.rs (3)
244-245: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd coverage for prefix invalidation.
This test exercises
clear()but never validates the newinvalidate_prefixbehavior. Add matching and non-matching keys, invalidate the prefix, and assert only the matching entries are removed.As per coding guidelines, every feature must have corresponding tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-core/src/cache/quick.rs` around lines 244 - 245, Add coverage in the cache test around clear() and cache.size() for invalidate_prefix: insert keys with both matching and non-matching prefixes, call cache.invalidate_prefix with the target prefix, and assert matching entries are removed while non-matching entries remain.Source: Coding guidelines
231-242: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStrengthen the eviction assertion.
size_bytes() <= CAPACITYandsize() < 258also pass if every oversized insertion is rejected. Assert that several fill entries were admitted, then verify the final resident weight/count is below the attempted total.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-core/src/cache/quick.rs` around lines 231 - 242, Strengthen the assertions in the cache fill test by verifying that multiple fill entries were admitted, rather than allowing all insertions to be rejected. After the insertion loop, assert a meaningful lower bound on resident entries or weight, then retain checks that size_bytes() stays within CAPACITY and the final count remains below the attempted total.
216-229: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the single-flight test concurrent.
The two calls are sequential, so the second call hits the value already inserted by the first and does not exercise the guard/API’s concurrent single-flight behavior. Start both
get_or_insert_with_keycalls in parallel before either loader can insert, then assertloads == 1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-core/src/cache/quick.rs` around lines 216 - 229, Make the test around get_or_insert_with_key concurrent by creating and starting both async calls before awaiting either result, using the shared loads counter and a synchronization mechanism if needed to overlap the loaders. Await both results, verify each returns vec![7u8], and retain the assertion that loads equals 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@rust/lance-core/src/cache/quick.rs`:
- Around line 244-245: Add coverage in the cache test around clear() and
cache.size() for invalidate_prefix: insert keys with both matching and
non-matching prefixes, call cache.invalidate_prefix with the target prefix, and
assert matching entries are removed while non-matching entries remain.
- Around line 231-242: Strengthen the assertions in the cache fill test by
verifying that multiple fill entries were admitted, rather than allowing all
insertions to be rejected. After the insertion loop, assert a meaningful lower
bound on resident entries or weight, then retain checks that size_bytes() stays
within CAPACITY and the final count remains below the attempted total.
- Around line 216-229: Make the test around get_or_insert_with_key concurrent by
creating and starting both async calls before awaiting either result, using the
shared loads counter and a synchronization mechanism if needed to overlap the
loaders. Await both results, verify each returns vec![7u8], and retain the
assertion that loads equals 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 948ce55b-aada-4e4e-97b5-481565874521
📒 Files selected for processing (2)
rust/lance-core/src/cache/moka.rsrust/lance-core/src/cache/quick.rs
One big tradeoff for quick_cache is shard count .
Mini benchmark for picking shard numberCell value: throughput in Mops = million cache get operations per second
Columns (ratio = cache capacity ÷ working-set size — how much of the data fits): 1 worker x 2 GiB (5s)
1 worker x 4 GiB (5s)
4 workers x 8 GiB (s1-s16: 5s)
4 workers x 16 GiB (s1-s16: 5s)
32 workers x 64 GiB
32 workers x 128 GiB
128 workers x 256 GiB
128 workers x 512 GiB
320 workers x 640 GiB
320 workers x 1280 GiB (s128-s1024: 5s, with power-of-two rounding proof)
Best shard number in my mindCPU = 1, shard = 1 AlgorithmWhat the formula yields
Wasted cache space when the working set does NOT fit (chosen shard counts)Waste = 100% − utilization: capacity the cache failed to fill while under eviction pressure (working set 10× / 2× larger than the cache). moka's global pool wastes ~0%; this is the entire space cost of sharding. waste ratio is not that big, acceptable |
|
End-to-end FTS shard sweep (real 100M-doc 42-language v3 index, 960 GiB cache, prewarmed, c128, 90s/arm) |
1 participant
Problem
moka records every cache hit into a single global read-op channel (input for its LRU/TinyLFU policy), and readers trigger inline housekeeping when the channel passes its flush point. Each warm hit therefore touches globally-shared cache lines (
record_read_op: channel length check + housekeeper try-lock CAS + try_send). At index-cache read rates this collapses:Pure
getmicrobenchmark, 50K entries x 4KB, 100% hits, 320-core host:A warm fts query performs ~2000 cache reads (posting groups + row-id columns across
350 partitions), so moka's ceiling imposes a hard qps cap. Background+7%): at these rates the 384-slot channel refills past its 64-entry flush point every ~0.13 ms, and the per-read atomics remain. This is the known, still-open moka issue moka-rs/moka#498.run_pending_tasksdraining was tested and does not help (Change
An opt-in
CacheBackendbacked by quick_cache, whose S3-FIFO/CLOCK-family policy records a hit by setting one reference bit on the entry itself — no read log, no reader-triggered housekeeping. Single-flightget_or_insertmaps toget_value_or_guard_async; weighted capacity maps toWeighter(u64 weights).LANCE_CACHE_BACKEND=quickopts in; moka remains the default.Results on a real workload
100M-doc 42-language corpus, 5-term
match_any(OR), k=100, warm + prewarmed, 320-core node, exact scoring, identical results; same binary (with #7950's chunked search), only the backend differs:The moka column inverts with concurrency (contention); the quick_cache column peaks at 93% CPU doing scoring work.
Semantics and scope
Tests
New backend test exercises roundtrip, single-flight load dedup, weighted-capacity enforcement under overfill, and clear, through the standard
LanceCacheAPI.cargo test -p lance-coregreen.Shard sizing (second commit)
quick_cache splits its weight budget evenly across shards with no cross-shard borrowing, and an entry heavier than ~its shard's share is silently refused admission (maintainer-confirmed behavior, arthurprs/quick-cache#55). Its default shard count (
available_parallelism × 4, rounded up to a power of two — 2048 on a 320-core host) is tuned for many-small-entry workloads; index-cache entries are MB-to-hundreds-of-MB posting groups, so small shares risk silent rejection and per-shard imbalance waste.The backend now derives the shard count as
min(cpus / 2, capacity / 4 GiB), rounded down to a power of two and clamped to [1, 1024]. The cpu term bounds lock contention (more shards than concurrent threads buys nothing), and the capacity term keeps every shard's share >= 4 GiB so the largest index-cache entries stay admissible — the same shape as RocksDB's block-cache default (capacity / min_shard_size, capped).Shard-count sweep (pure get, 50K × 4KB entries, 320-core host, 256 readers) shows contention is a non-issue at low shard counts — even a single-shard quick_cache is 10x moka, and throughput plateaus from ~64 shards:
On a 320-core host with 960 GiB capacity this yields 128 shards (7.5 GiB share, on the throughput plateau); a 4-core host with 8 GiB yields 2 shards (4 GiB share) — entries up to ~3.9 GiB stay admissible everywhere. A standalone matrix bench across machine profiles (workers 1-320, capacities 2 GiB-1.25 TiB, cache/working-set ratios 0.1-2.0, lance-style 90% 1 MiB + 10% 128 MiB entry mix) confirms every landing point clears the measured demand (~3 Mops of cache gets at 1300 qps FTS) by 2-10x under eviction pressure and 20-100x warm, while moka baselines sit at 0.2-4.7 Mops and degrade as workers grow.
End-to-end validation (real 100M-doc 42-language FTS index, prewarmed, c128, 90s/arm, 960 GiB cache): shard counts 1 / 16 / 64 / 128 / 512 / 2048 all measure 1178-1323 qps — inside run-to-run noise — versus ~180 qps on moka. Warm hits take no lock (atomic CLOCK bit), so shard count only affects admission and eviction-phase contention; sizing by admission margin is free. The formula also auto-sized the co-resident 36 GiB metadata cache to 8 shards.