clearing cache with pageout#8654
Conversation
ee15a2e to
b9392c1
Compare
dee3b0e to
3d47d10
Compare
3d47d10 to
5816133
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
lib/gridstore/src/gridstore/mod.rs (1)
516-529: Update the method doc wording to match behavior.The method now clears mmap/page cache via underlying components, so “Drop disk cache” is misleading. Consider “Drop RAM page cache” (or similar) for accuracy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/gridstore/src/gridstore/mod.rs` around lines 516 - 529, The docstring for the clear_cache method is inaccurate: it currently reads "Drop disk cache" but the implementation clears in-memory mmap/page caches via pages.read().clear_cache() and bitmask.read().clear_cache(); update the method doc comment to reflect that it drops the RAM/page (mmap) cache (e.g., "Drop RAM page cache" or "Clear in-memory mmap/page cache") so the documentation matches the behavior of clear_cache.lib/sparse/src/index/inverted_index/inverted_index_compressed_immutable_ram.rs (1)
60-60: Consider making cache eviction best-effort inopen()(Line 60).While
clear_cache()currently always succeeds (advisory madvise failures are logged internally at the advice layer), thestd::io::Result<()>return type suggests it can fail. Since cache eviction is advisory and the in-memory data is already fully loaded, opening should not fail due to cache-hinting errors.♻️ Proposed change
- mmap_inverted_index.clear_cache()?; + if let Err(err) = mmap_inverted_index.clear_cache() { + log::warn!( + "Failed to clear mmap cache after loading immutable RAM inverted index: {err}" + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sparse/src/index/inverted_index/inverted_index_compressed_immutable_ram.rs` at line 60, The open() path currently calls mmap_inverted_index.clear_cache() and propagates its std::io::Result, which can cause open() to fail on advisory cache-hinting errors; change this to a best-effort call so open() never fails because of cache eviction. Update the open() implementation to call mmap_inverted_index.clear_cache() and ignore or log any Err (e.g., call .ok() or match and log a debug/warn via the existing logger) so failures are non-fatal, ensuring the rest of open() continues normally.lib/sparse/src/index/inverted_index/inverted_index_compressed_mmap.rs (1)
390-399: Minor style inconsistency in destructuring pattern.The field
_phantomuses a direct binding pattern while other fields use the: _pattern for explicit ignoring. For consistency within this destructuring block, consider using_phantom: _.Suggested fix for consistency
let Self { path: _, mmap, file_header: _, - _phantom, + _phantom: _, } = self;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sparse/src/index/inverted_index/inverted_index_compressed_mmap.rs` around lines 390 - 399, The destructuring in the clear_cache method uses mixed ignore patterns (path: _, file_header: _, but _phantom); change the `_phantom` binding to the consistent `_phantom: _` pattern in the `let Self { path: _, mmap, file_header: _, _phantom, } = self;` destructure inside the `pub fn clear_cache(&self)` so all ignored fields use the `: _` form while leaving `mmap` as-is; this keeps style consistent for the struct fields in the clear_cache function.lib/segment/src/vector_storage/chunked_vectors.rs (1)
406-415: Theheap_size_bytesmethod always returns 0.For mmap-backed
ChunkedVectors, this is likely intentional since the actual data resides in memory-mapped files rather than heap-allocated memory. The destructuring pattern ensures this method will be revisited if new heap-allocated fields are added. If this is the intended behavior, consider adding a brief comment explaining why the heap size is zero for mmap-backed storage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/segment/src/vector_storage/chunked_vectors.rs` around lines 406 - 415, The heap_size_bytes method in ChunkedVectors always returns 0 which is confusing; either document that mmap-backed storage has no heap-allocated payload or implement a proper calculation for any heap-owned fields. Update the heap_size_bytes function (in struct ChunkedVectors) to include a short comment explaining that data is memory-mapped so heap usage is zero for current fields, or, if you prefer to compute actual heap usage, iterate and sum sizes of heap-allocated members (e.g., any Vec/Box/String in config, status, chunks, directory) and return that total instead of 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/common/common/src/mmap/advice.rs`:
- Around line 145-170: The trait method clear_cache currently returns () but
callers (e.g., MmapSlice, MmapBitSlice, MmapSliceReadOnly) use the ? operator,
so change clear_cache(&self) -> io::Result<()> and update its linux-specific
body to propagate errors from pageout_impl (make pageout_impl(&self) ->
io::Result<()> as well) instead of swallowing them; likewise update
pageout_slice() to return io::Result<()> and surface any madvise failures, and
then update implementations of pageout_impl/pageout_slice to return appropriate
io::Result values so callers can use ? correctly.
---
Nitpick comments:
In `@lib/gridstore/src/gridstore/mod.rs`:
- Around line 516-529: The docstring for the clear_cache method is inaccurate:
it currently reads "Drop disk cache" but the implementation clears in-memory
mmap/page caches via pages.read().clear_cache() and
bitmask.read().clear_cache(); update the method doc comment to reflect that it
drops the RAM/page (mmap) cache (e.g., "Drop RAM page cache" or "Clear in-memory
mmap/page cache") so the documentation matches the behavior of clear_cache.
In `@lib/segment/src/vector_storage/chunked_vectors.rs`:
- Around line 406-415: The heap_size_bytes method in ChunkedVectors always
returns 0 which is confusing; either document that mmap-backed storage has no
heap-allocated payload or implement a proper calculation for any heap-owned
fields. Update the heap_size_bytes function (in struct ChunkedVectors) to
include a short comment explaining that data is memory-mapped so heap usage is
zero for current fields, or, if you prefer to compute actual heap usage, iterate
and sum sizes of heap-allocated members (e.g., any Vec/Box/String in config,
status, chunks, directory) and return that total instead of 0.
In
`@lib/sparse/src/index/inverted_index/inverted_index_compressed_immutable_ram.rs`:
- Line 60: The open() path currently calls mmap_inverted_index.clear_cache() and
propagates its std::io::Result, which can cause open() to fail on advisory
cache-hinting errors; change this to a best-effort call so open() never fails
because of cache eviction. Update the open() implementation to call
mmap_inverted_index.clear_cache() and ignore or log any Err (e.g., call .ok() or
match and log a debug/warn via the existing logger) so failures are non-fatal,
ensuring the rest of open() continues normally.
In `@lib/sparse/src/index/inverted_index/inverted_index_compressed_mmap.rs`:
- Around line 390-399: The destructuring in the clear_cache method uses mixed
ignore patterns (path: _, file_header: _, but _phantom); change the `_phantom`
binding to the consistent `_phantom: _` pattern in the `let Self { path: _,
mmap, file_header: _, _phantom, } = self;` destructure inside the `pub fn
clear_cache(&self)` so all ignored fields use the `: _` form while leaving
`mmap` as-is; this keeps style consistent for the struct fields in the
clear_cache function.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e3e18c4-f029-4394-b5f1-b263321f36f9
📒 Files selected for processing (39)
lib/common/common/src/mmap/advice.rslib/common/common/src/mmap/mmap_readonly.rslib/common/common/src/mmap/mmap_rw.rslib/common/common/src/mmap_hashmap.rslib/common/common/src/universal_io/mmap.rslib/common/common/src/universal_io/read.rslib/gridstore/src/bitmask/gaps.rslib/gridstore/src/bitmask/mod.rslib/gridstore/src/gridstore/mod.rslib/gridstore/src/gridstore/reader.rslib/gridstore/src/pages.rslib/segment/src/common/flags/bitvec_flags.rslib/segment/src/common/flags/buffered_dynamic_flags.rslib/segment/src/common/flags/dynamic_mmap_flags.rslib/segment/src/common/flags/roaring_flags.rslib/segment/src/common/mmap_bitslice_buffered_update_wrapper.rslib/segment/src/index/field_index/full_text_index/inverted_index/mmap_inverted_index/mmap_postings.rslib/segment/src/index/field_index/full_text_index/inverted_index/mmap_inverted_index/mmap_postings_enum.rslib/segment/src/index/field_index/full_text_index/inverted_index/mmap_inverted_index/mod.rslib/segment/src/index/field_index/geo_index/mmap_geo_index.rslib/segment/src/index/field_index/map_index/mmap_map_index.rslib/segment/src/index/field_index/numeric_index/mmap_numeric_index.rslib/segment/src/index/field_index/stored_point_to_values.rslib/segment/src/index/hnsw_index/graph_layers.rslib/segment/src/index/hnsw_index/graph_links.rslib/segment/src/index/hnsw_index/hnsw.rslib/segment/src/vector_storage/chunked_vectors.rslib/segment/src/vector_storage/dense/dense_vector_storage.rslib/segment/src/vector_storage/dense/immutable_dense_vectors.rslib/segment/src/vector_storage/multi_dense/appendable_mmap_multi_dense_vector_storage.rslib/segment/src/vector_storage/quantized/quantized_chunked_mmap_storage.rslib/segment/src/vector_storage/quantized/quantized_mmap_storage.rslib/segment/src/vector_storage/quantized/quantized_multivector_storage.rslib/segment/src/vector_storage/quantized/quantized_vectors.rslib/segment/src/vector_storage/sparse/mmap_sparse_vector_storage.rslib/sparse/src/index/inverted_index/inverted_index_compressed_immutable_ram.rslib/sparse/src/index/inverted_index/inverted_index_compressed_mmap.rslib/sparse/src/index/inverted_index/inverted_index_immutable_ram.rslib/sparse/src/index/inverted_index/inverted_index_mmap.rs
| fn clear_cache(&self) { | ||
| #[cfg(target_os = "linux")] | ||
| { | ||
| use std::sync::LazyLock; | ||
|
|
||
| /// True if `MADV_PAGEOUT` is supported (added in Linux 5.4). | ||
| /// Probed by calling `madvise` with a zero-length range, which | ||
| /// validates the advice value without touching any memory. | ||
| /// | ||
| /// As shown in madvise man pages: | ||
| /// > `madvise(0, 0, advice)` will return zero iff advice is supported by the kernel | ||
| /// > and can be relied on to probe for support. | ||
| static PAGEOUT_IS_SUPPORTED: LazyLock<bool> = LazyLock::new(|| { | ||
| let res = | ||
| unsafe { nix::libc::madvise(std::ptr::null_mut(), 0, nix::libc::MADV_PAGEOUT) }; | ||
| res == 0 | ||
| }); | ||
|
|
||
| if *PAGEOUT_IS_SUPPORTED { | ||
| self.pageout_impl(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(target_os = "linux")] | ||
| fn pageout_impl(&self); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Trait/root implementation =="
sed -n '139,245p' lib/common/common/src/mmap/advice.rs
echo
echo "== Writable wrappers exposing Result =="
sed -n '186,205p' lib/common/common/src/mmap/mmap_rw.rs
sed -n '311,321p' lib/common/common/src/mmap/mmap_rw.rs
sed -n '430,440p' lib/common/common/src/mmap/mmap_rw.rs
echo
echo "== Read-only wrappers exposing Result =="
sed -n '150,160p' lib/common/common/src/mmap/mmap_readonly.rs
sed -n '349,359p' lib/common/common/src/mmap/mmap_readonly.rs
echo
echo "== Call sites that currently use ? and therefore expect propagation =="
rg -n --type=rust '\.clear_cache\(\)\?' lib/common lib/segmentRepository: qdrant/qdrant
Length of output: 14399
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Exact trait method signature ==="
sed -n '145p' lib/common/common/src/mmap/advice.rs
echo
echo "=== Full trait definition (method only) ==="
rg -A 2 'fn clear_cache\(' lib/common/common/src/mmap/advice.rs | head -20
echo
echo "=== Check which wrappers use ? on clear_cache() ==="
rg -B 2 -A 2 'clear_cache\(\)\?' lib/common/common/src/mmap/mmap_rw.rs | head -40
echo
echo "=== Check which wrappers do NOT use ? ==="
rg -B 2 -A 2 'clear_cache\(\);' lib/common/common/src/mmap/mmap_rw.rs | head -40Repository: qdrant/qdrant
Length of output: 766
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check for compilation errors in mmap_rw.rs ==="
cd lib/common/common
cargo check 2>&1 | grep -A 5 "clear_cache" || echo "No clear_cache errors found or cargo check passed"
echo
echo "=== List all files with clear_cache calls and their patterns ==="
rg -l 'clear_cache' lib/common lib/segment 2>/dev/null | while read f; do
echo "File: $f"
rg -n 'clear_cache\(\)' "$f" | head -3
doneRepository: qdrant/qdrant
Length of output: 223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full cargo check output ==="
cd lib/common/common
cargo check 2>&1 | head -100
echo
echo "=== Lines from mmap_rw.rs with clear_cache and surrounding context ==="
sed -n '185,210p' lib/common/common/src/mmap/mmap_rw.rs
echo
echo "=== Lines from mmap_readonly.rs with clear_cache and surrounding context ==="
sed -n '149,165p' lib/common/common/src/mmap/mmap_readonly.rsRepository: qdrant/qdrant
Length of output: 335
🏁 Script executed:
#!/bin/bash
set -euoripefail
echo "=== Trait signature in advice.rs (exact lines) ==="
cat -n lib/common/common/src/mmap/advice.rs | sed -n '145,150p'
echo
echo "=== Wrapper implementations in mmap_rw.rs (exact lines around 186-210) ==="
cat -n lib/common/common/src/mmap/mmap_rw.rs | sed -n '186,210p'
echo
echo "=== Wrapper implementations in mmap_readonly.rs (exact lines around 150-165) ==="
cat -n lib/common/common/src/mmap/mmap_readonly.rs | sed -n '150,165p'
echo
echo "=== Check if any const/static trait impl exists with different signature ==="
rg -n 'trait Madviseable' lib/common/common/src/mmap/advice.rs
echo
echo "=== Full trait block ==="
sed -n '100,200p' lib/common/common/src/mmap/advice.rs | grep -n 'fn clear_cache' -A 5 -B 2Repository: qdrant/qdrant
Length of output: 731
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Trait signature in advice.rs (exact lines) ==="
cat -n lib/common/common/src/mmap/advice.rs | sed -n '145,150p'
echo
echo "=== Wrapper implementations in mmap_rw.rs (exact lines around 186-210) ==="
cat -n lib/common/common/src/mmap/mmap_rw.rs | sed -n '186,210p'
echo
echo "=== Wrapper implementations in mmap_readonly.rs (exact lines around 150-165) ==="
cat -n lib/common/common/src/mmap/mmap_readonly.rs | sed -n '150,165p'
echo
echo "=== Full trait block with method signatures ==="
sed -n '100,180p' lib/common/common/src/mmap/advice.rs | cat -nRepository: qdrant/qdrant
Length of output: 5461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check lines 311-325 in mmap_rw.rs ==="
cat -n lib/common/common/src/mmap/mmap_rw.rs | sed -n '310,330p'
echo
echo "=== Check lines 430-450 in mmap_rw.rs ==="
cat -n lib/common/common/src/mmap/mmap_rw.rs | sed -n '430,450p'
echo
echo "=== Check lines 349-365 in mmap_readonly.rs ==="
cat -n lib/common/common/src/mmap/mmap_readonly.rs | sed -n '349,365p'Repository: qdrant/qdrant
Length of output: 1976
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify no other trait definitions exist (avoid false positives) ==="
rg -n 'trait Madviseable' lib/common/common/src/mmap/
echo
echo "=== Count wrapper methods with ? vs without ? on clear_cache ==="
echo "With ? operator:"
rg -c 'mmap\.clear_cache\(\)\?' lib/common/common/src/mmap/mmap_rw.rs lib/common/common/src/mmap/mmap_readonly.rs || echo "0"
echo "Without ? operator:"
rg -c 'mmap\.clear_cache\(\);' lib/common/common/src/mmap/mmap_rw.rs lib/common/common/src/mmap/mmap_readonly.rs || echo "0"
echo
echo "=== Verify this is the only Madviseable trait (no hidden defs) ==="
rg -l 'fn clear_cache' lib/common lib/segment | head -20Repository: qdrant/qdrant
Length of output: 1746
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== All wrapper clear_cache methods with context ==="
rg -B 3 -A 3 'pub fn clear_cache' lib/common/common/src/mmap/mmap_rw.rs lib/common/common/src/mmap/mmap_readonly.rs
echo
echo "=== Summary: type signatures of each wrapper ==="
echo "File: mmap_rw.rs - MmapType wrapper"
sed -n '192,196p' lib/common/common/src/mmap/mmap_rw.rs
echo
echo "File: mmap_rw.rs - MmapSlice wrapper"
sed -n '317,321p' lib/common/common/src/mmap/mmap_rw.rs
echo
echo "File: mmap_rw.rs - MmapBitSlice wrapper"
sed -n '436,440p' lib/common/common/src/mmap/mmap_rw.rs
echo
echo "File: mmap_readonly.rs - MmapTypeReadOnly wrapper"
sed -n '156,160p' lib/common/common/src/mmap/mmap_readonly.rs
echo
echo "File: mmap_readonly.rs - MmapSliceReadOnly wrapper"
sed -n '355,359p' lib/common/common/src/mmap/mmap_readonly.rs
echo
echo "=== Trait definition ==="
sed -n '40,68p' lib/common/common/src/mmap/advice.rsRepository: qdrant/qdrant
Length of output: 4392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Exact trait method signature for clear_cache ==="
sed -n '40,68p' lib/common/common/src/mmap/advice.rs | cat -n
echo
echo "=== Check pageout_slice return type ==="
sed -n '74,95p' lib/common/common/src/mmap/advice.rs | cat -n
echo
echo "=== Check if pageout_impl trait method has return type ==="
sed -n '70,72p' lib/common/common/src/mmap/advice.rsRepository: qdrant/qdrant
Length of output: 1981
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full trait Madviseable definition ==="
sed -n '99,180p' lib/common/common/src/mmap/advice.rs | cat -n
echo
echo "=== Verify pageout_slice does not return Result ==="
rg -A 20 'fn pageout_slice' lib/common/common/src/mmap/advice.rs | head -30Repository: qdrant/qdrant
Length of output: 4235
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check which wrapper types are actually exported and used ==="
rg -n 'pub struct MmapSlice' lib/common/common/src/mmap/mmap_rw.rs
echo
echo "=== Check which wrapper types are actually exported and used ==="
rg -n 'pub struct MmapBitSlice' lib/common/common/src/mmap/mmap_rw.rs
echo
echo "=== List all public wrapper types in mmap_rw.rs ==="
rg -n 'pub struct' lib/common/common/src/mmap/mmap_rw.rs | head -20
echo
echo "=== Verify the code at MmapSlice clear_cache has the actual problematic ? ==="
sed -n '305,325p' lib/common/common/src/mmap/mmap_rw.rsRepository: qdrant/qdrant
Length of output: 952
Fix type mismatch: clear_cache() trait returns () but wrappers use ? operator on it (compile error).
The trait method clear_cache(&self) returns () (lines 47–69), but wrappers like MmapSlice, MmapBitSlice, and MmapSliceReadOnly call mmap.clear_cache()? with the try operator (lines 319, 438, 357), which only works on Result types. This causes a compile error. Additionally, pageout_slice() logs failures silently instead of propagating them, so the API can claim success without clearing anything.
Either make the trait return io::Result<()> and propagate errors from pageout_impl()/pageout_slice(), or mark all clear_cache() methods as best-effort advisory and remove the ? operators from wrappers. Given that downstream call sites expect propagation (see vector_storage_base.rs:466–489 and others), returning Result is the correct approach.
Suggested fix
- fn clear_cache(&self) {
+ fn clear_cache(&self) -> io::Result<()> {
#[cfg(target_os = "linux")]
{
use std::sync::LazyLock;
static PAGEOUT_IS_SUPPORTED: LazyLock<bool> = LazyLock::new(|| {
let res =
unsafe { nix::libc::madvise(std::ptr::null_mut(), 0, nix::libc::MADV_PAGEOUT) };
res == 0
});
if *PAGEOUT_IS_SUPPORTED {
- self.pageout_impl();
+ self.pageout_impl()?;
}
}
+ Ok(())
}
#[cfg(target_os = "linux")]
- fn pageout_impl(&self);
+ fn pageout_impl(&self) -> io::Result<()>;
}
#[cfg(target_os = "linux")]
-fn pageout_slice(slice: &[u8]) {
+fn pageout_slice(slice: &[u8]) -> io::Result<()> {
if slice.is_empty() {
- return;
+ return Ok(());
}
let res = unsafe {
nix::libc::madvise(
slice.as_ptr() as *mut _,
slice.len(),
nix::libc::MADV_PAGEOUT,
)
};
if res != 0 {
- let err = io::Error::last_os_error();
- log::warn!("Failed to call madvise(MADV_PAGEOUT): {err}");
+ return Err(io::Error::last_os_error());
}
+ Ok(())
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/common/common/src/mmap/advice.rs` around lines 145 - 170, The trait
method clear_cache currently returns () but callers (e.g., MmapSlice,
MmapBitSlice, MmapSliceReadOnly) use the ? operator, so change
clear_cache(&self) -> io::Result<()> and update its linux-specific body to
propagate errors from pageout_impl (make pageout_impl(&self) -> io::Result<()>
as well) instead of swallowing them; likewise update pageout_slice() to return
io::Result<()> and surface any madvise failures, and then update implementations
of pageout_impl/pageout_slice to return appropriate io::Result values so callers
can use ? correctly.
* use pageout to clear mmap cache * Also clear cache of deleted flags in mmap dense vector storage * Add reference to madvise man pages for probe logic * use deconstruct --------- Co-authored-by: timvisee <[email protected]>
Depends on #8606