Take into account key size for MemorySizedCache - #6720
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes MemorySizedCache under-accounting by charging cache entries for both key + value memory, addressing the reported key-driven memory growth (notably for LeafSearchCache where keys embed large SearchRequests).
Changes:
- Introduces a
MemUsagetrait (plus helpers) and implements it for relevant cache key types (SliceAddress,LeafSearchCache::CacheKey, plus foundational impls likeString/ tuples). - Updates the underlying cache backends (LRU / S3Fifo / TinyLfu) to enforce capacity and metrics using entry size = key_mem_usage + value_len.
- Adjusts tests and documentation to reflect “keys included” cache capacity semantics.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| quickwit/quickwit-storage/src/lib.rs | Re-exports MemUsage and owned_mem_usage from the storage crate surface. |
| quickwit/quickwit-storage/src/cache/stored_item.rs | Adds KeyedEntry and tracks per-entry key memory to keep eviction paths cheap and accounting consistent. |
| quickwit/quickwit-storage/src/cache/slice_address.rs | Implements MemUsage for SliceAddress so slice caches can charge keys. |
| quickwit/quickwit-storage/src/cache/mod.rs | Adds the new mem_usage module and re-exports its API. |
| quickwit/quickwit-storage/src/cache/memory_sized_cache.rs | Requires K: MemUsage and expands tests to validate key charging across policies. |
| quickwit/quickwit-storage/src/cache/mem_usage.rs | New MemUsage trait + helper and unit tests. |
| quickwit/quickwit-storage/src/cache/base_cache.rs | Makes LRU/S3Fifo/TinyLfu capacity and metrics account for key size via stored key footprint. |
| quickwit/quickwit-search/src/leaf_cache.rs | Implements MemUsage for LeafSearchCache keys using SearchRequest::encoded_len() as a proxy. |
| quickwit/quickwit-config/src/node_config/mod.rs | Adds CacheConfig::with_capacity_and_policy helper (used in tests). |
| docs/configuration/node-config.md | Documents that cache capacities bound key+value, not just value bytes. |
Suppressed comments (2)
quickwit/quickwit-storage/src/cache/base_cache.rs:358
- This warning message is now also triggered when the key makes the entry exceed capacity (even if the value itself is small), but the message still says "byte slice". Updating the wording will make logs less misleading.
capacity_in_bytes = ?self.capacity,
len = value.len(),
key_mem_usage,
"Downloaded a byte slice larger than the cache capacity."
);
quickwit/quickwit-storage/src/cache/base_cache.rs:479
- This warning message is now also triggered when the key makes the entry exceed capacity (even if the value itself is small), but the message still says "byte slice". Updating the wording will make logs less misleading.
capacity_in_bytes = ?self.capacity,
len = value.len(),
key_mem_usage,
"Downloaded a byte slice larger than the cache capacity."
);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
18157a7 to
4c70b02
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5dfaaf9374
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Description
Take into account key size for
MemorySizedCache; this is particularly problematic forLeafSearchCachewhich uses the (large)SearchRequest, including the whole query, as a cache key and for queries that mostly return zero-hit splits.Attempt to fix #6719.
MemUsagetraitMemUsageforCacheKeyandSliceAddressmem_usagewhen inserting and reporting the cachein_cache_num_bytesandevict_num_byteshits_num_bytesstill return the value size only, without taking into account the key size.How was this PR tested?
Unit tests only.