fix(index): exclude shared metadata cache from LanceIndexStore deep size#7574
Conversation
`LanceIndexStore::deep_size_of_children` summed its `metadata_cache`, but that cache is session-scoped and shared: `Session::deep_size_of_children` already accounts for it once, and `LanceCache`'s size ignores the dedup `Context`. So every opened index holding a store re-counted the entire cache as its own bytes, inflating and N-times double-counting reported memory. Count only the store's own fields (object store + index dir). Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| // Exclude the shared, session-scoped `metadata_cache` (accounted once by | ||
| // `Session::deep_size_of_children`): it is not this store's own footprint, and | ||
| // sizing it here makes every opened index that holds a store report the whole | ||
| // cache as its own bytes — inflating and N-times double-counting it. |
There was a problem hiding this comment.
issue(non-blocking): Removing metadata_cache seems correct, but object store is another shared object here. I wonder if the real fix is making LanceIndexStore not part of the cache entries themselves, instead just making it cheap to reconstruct LanceIndexStore on load. You can see how we did this for Vector indices with IvfStateEntry:
lance/rust/lance/src/index/vector/ivf/v2.rs
Lines 228 to 238 in 71c4aa2
There was a problem hiding this comment.
Thanks Will! I'll take a look and follow up
There was a problem hiding this comment.
hmm, I took a look, so you're saying we should remove LanceIndexStore from all the scalar indexes so that we're not caching it and instead it's always reconstructed? Similar to what some are already doing with their *State structs?
There was a problem hiding this comment.
I can enumerate the scalar indexes which don't currently have a *State + reconstruct impls
There was a problem hiding this comment.
Is this general direction good with you Will? #7652
If so, I can create PR's for the other indexes as well and then drop the LanceIndexStore.
Summary
LanceIndexStore::deep_size_of_childrencounted itsmetadata_cache:But that cache is session-scoped and shared.
Session::deep_size_of_childrenalready accounts for it once (rust/lance/src/session.rs), andLanceCache::deep_size_of_childrenreturnsapprox_size_bytes()while ignoring the dedupContext. So the shared cache isn't deduplicated: every opened index that holds a store re-counts the entire cache as its own bytes, inflating and N-times double-counting reported memory (N = number of open index stores).Change
Count only the store's own footprint — the object store and index dir. The shared metadata cache is left to
Sessionto report once.Test plan
test_store_deep_size_excludes_metadata_cache— inserting a 4 MB blob into the shared cache must not changestore.deep_size_of(). Verified it fails before the fix (before: 276, after: 4,194,620) and passes after.cargo fmt --allandcargo clippy -p lance-index --all-targets -- -D warningsclean.Made with Cursor