[improvement](be) Add stampede protection for AnnIndexIVFListCache#62442
Conversation
### What problem does this PR solve? Issue Number: close #xxx Problem Summary: When multiple threads concurrently miss the AnnIndexIVFListCache for the same IVF list key, each thread independently reads the same data from disk and inserts it into the cache. The LRU cache silently replaces duplicate entries (counted as "stampede"), causing: 1. Redundant disk I/O: N concurrent misses produce N identical reads instead of 1. 2. Ghost memory: replaced entries whose handles are still pinned by other threads consume memory that is no longer tracked by cache usage, leading to cache usage ratio fluctuations (observed 87%→50% under high concurrency). 3. Lower cache hit ratio: unnecessary evictions caused by the inflated insertion volume. ### Release note Reduced redundant disk I/O and memory waste for IVF on-disk vector index cache under concurrent queries by adding stampede protection (double-check locking pattern) to CachedRandomAccessReader::borrow(). ### Check List (For Author) - Test: Manual test with vectordb_bench concurrency benchmark - Behavior changed: No - Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…ampede protection
### What problem does this PR solve?
Issue Number: close #xxx
Problem Summary: The stampede protection double-check path in
CachedRandomAccessReader::borrow() (lines 372-375) had 0% test coverage.
Multiple threads concurrently missing the fast-path cache lookup would
all acquire the I/O mutex, but only the first thread performs disk I/O;
the remaining threads should hit the cache on re-check. This path was
never exercised by any existing test.
### Release note
None
### Check List (For Author)
- Test: Unit Test / Integration Test
- Cache-level unit tests (AnnIndexIVFListCacheTest): basic ops, miss paths,
multi-entry, stampede simulation (single & multi-key), singleton lifecycle
- Integration tests (VectorSearchTest): IVF_ON_DISK save/load/search and
concurrent search with 8 threads to exercise the actual borrow()
double-check cache hit path
- Behavior changed: No
- Does this need documentation: No
\ud83d\udc98 Generated with Crush
Assisted-by: Claude Opus 4.6 via Crush <crush@charm.land>
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Problem Summary: The concurrent stampede protection test created separate FaissVectorIndex instances per thread, each with its own CachedRandomAccessReader and _io_mutex. This meant threads never contended on the same mutex, so the double-check cache hit path (faiss_ann_index.cpp:373-375) was never exercised. Fix: all threads now share a single FaissVectorIndex instance so they contend on the same _io_mutex, allowing the double-check lookup to find entries cached by a preceding thread. ### Release note None ### Check List (For Author) - Test: Unit Test - Behavior changed: No - Does this need documentation: No \ud83d\udc98 Generated with Crush Assisted-by: Claude Opus 4.6 via Crush <crush@charm.land>
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
{
"overall": "No blocking issues found. The patch correctly narrows the cache stampede window in CachedRandomAccessReader::borrow() by rechecking the shared AnnIndexIVFListCache under the existing _io_mutex, and the added tests are focused on the affected behavior.",
"findings": [],
"critical_checkpoints": {
"goal_and_proof": "Pass. The goal is to avoid redundant IVF_ON_DISK list reads for concurrent misses on the same reader and add coverage for that path. borrow() now rechecks the cache while holding _io_mutex before doing disk I/O, and targeted BE unit and integration tests were added.",
"minimality": "Pass. The functional change is localized to the miss path in CachedRandomAccessReader plus focused cache and ANN tests.",
"concurrency": "Pass. The concurrent actors are query threads sharing one CachedRandomAccessReader. The stateful CLucene IndexInput remains protected by _io_mutex. The added locked work is only a second cache lookup before the existing read path, and I did not find lock order or deadlock issues.",
"lifecycle_and_static_init": "Pass. Cache handle ownership remains RAII-based through PageCacheHandle, cache values still inherit LRUCacheValueBase, and the change introduces no new static initialization dependency.",
"configuration": "Not applicable. No new config was added.",
"compatibility": "Not applicable. No protocol, symbol, or storage format change was introduced.",
"parallel_paths": "Pass. The change applies to the IVF_ON_DISK RandomAccessReader miss path, which is the relevant parallel read path for this cache behavior.",
"conditional_checks": "Pass. The double-check branch is documented in code and matches the surrounding CLucene synchronization model.",
"test_coverage": "Pass with note. Added cache-level unit tests and IVF_ON_DISK integration tests. The public macOS BE UT failure is an environment issue (runner JDK 25 vs required JDK 17), not a code failure from this PR.",
"observability": "Pass. Existing hit, miss, and fetch-page metrics are preserved and remain sufficient for diagnosing this path.",
"transaction_and_persistence": "Not applicable. No transaction, persistence, or edit log logic was changed.",
"data_writes": "Not applicable. This PR only changes the read-side on-disk IVF cache path.",
"fe_be_variable_passing": "Not applicable. No FE/BE interface changes were introduced.",
"performance": "Pass. The cache-hit fast path stays lock-free, and the miss path now avoids redundant disk reads and duplicate cache insertions under contention."
},
"residual_risks": [
"The coverage is BE-unit focused. There is no regression-test suite change because the behavior is internal to the BE IVF_ON_DISK cache path."
],
"decision": "approve"
}
…62442) ## Summary - Add double-check locking pattern to `CachedRandomAccessReader::borrow()` to prevent concurrent threads from redundantly reading the same IVF list data from disk (cache stampede) - Eliminate ghost memory caused by LRU cache entry replacement under high concurrency, reducing usage ratio fluctuations - Zero overhead on the fast path (cache hit); reuses the existing `_io_mutex` without introducing new synchronization primitives ## Proposed Changes When multiple threads concurrently miss the `AnnIndexIVFListCache` for the same key, they all independently read the same data from disk and insert it into the cache. The LRU cache silently replaces duplicate entries (counted as "stampede"), causing: 1. **Redundant disk I/O**: N concurrent misses produce N identical reads instead of 1 2. **Ghost memory**: replaced entries whose handles are still pinned consume memory not tracked by cache `_usage`, leading to usage ratio fluctuations (observed 87%→50% under high concurrency) 3. **Lower cache hit ratio**: unnecessary evictions caused by inflated insertion volume The fix leverages the existing `_io_mutex` (required because CLucene `IndexInput` is stateful) by moving its acquisition before the disk read and adding a cache re-check after acquiring the lock. If a preceding thread already loaded the same key, subsequent threads skip the disk I/O entirely. ### What problem does this PR solve? Problem Summary: Under high concurrency (e.g., vectordb_bench with 10-90 concurrent threads), the AnnIndexIVFListCache exhibits excessive stampede events, redundant disk I/O, and usage ratio fluctuations due to the TOCTOU gap between cache lookup and insert. ### Release note Reduced redundant disk I/O and memory waste for IVF on-disk vector index cache under concurrent queries by adding stampede protection (double-check locking pattern) to CachedRandomAccessReader::borrow(). ### Check List (For Author) - Test: Manual test with vectordb_bench concurrency benchmark - Behavior changed: No - Does this need documentation: No
3 participants
Summary
CachedRandomAccessReader::borrow()to prevent concurrent threads from redundantly reading the same IVF list data from disk (cache stampede)_io_mutexwithout introducing new synchronization primitivesProposed Changes
When multiple threads concurrently miss the
AnnIndexIVFListCachefor the same key, they all independently read the same data from disk and insert it into the cache. The LRU cache silently replaces duplicate entries (counted as "stampede"), causing:_usage, leading to usage ratio fluctuations (observed 87%→50% under high concurrency)The fix leverages the existing
_io_mutex(required because CLuceneIndexInputis stateful) by moving its acquisition before the disk read and adding a cache re-check after acquiring the lock. If a preceding thread already loaded the same key, subsequent threads skip the disk I/O entirely.What problem does this PR solve?
Problem Summary: Under high concurrency (e.g., vectordb_bench with 10-90 concurrent threads), the AnnIndexIVFListCache exhibits excessive stampede events, redundant disk I/O, and usage ratio fluctuations due to the TOCTOU gap between cache lookup and insert.
Release note
Reduced redundant disk I/O and memory waste for IVF on-disk vector index cache under concurrent queries by adding stampede protection (double-check locking pattern) to CachedRandomAccessReader::borrow().
Check List (For Author)