perf(io): batch cache misses in multiread

[LHT129]

Closes #2638\n\nSummary: deduplicate cached MultiRead pages and load owned misses through one backend MultiReadImpl call. Share in-flight page loads, prevent stale in-flight pages from entering the cache, and add coverage for batched misses, cross-page reads, concurrent loads, and failed-load retry.\n\nValidation: make fmt; make test CASE=[ReadCache]; make release.

[vsag-bot]

/label status/waiting-for-review
/waiting-on reviewer
/request-review @jiaweizone
/request-review @wxyucs
/request-review @inabao
/request-review @vsag-bot

[vsag-bot]

Automated pull request review failed.

Review effort: medium (281 changed lines across 2 files).

socket hang up

No GitHub review was submitted.

[mergify]

Merge Protections

🟢 All 2 merge protections satisfied — ready to merge.

Show 2 satisfied protections

🟢 Require kind label

• label~=^kind/

🟢 Require version label

• label~=^version/

[Copilot]

Pull request overview

This PR improves the BasicIO read-cache path for MultiRead by deduplicating page fetches, batching cache misses into a single backend MultiReadImpl call, and sharing in-flight page loads to avoid redundant backend reads under concurrency.

Changes:

• Updated BasicIO::MultiRead to precompute required page IDs, bulk-load missing pages, then serve reads from cached pages.
• Added in-flight page-load coordination (loading_pages_) to share concurrent misses and prevent stale in-flight pages from being inserted into the cache.
• Added unit tests covering batched misses, cross-page reads, concurrent shared loads, and retry after failed load.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/io/common/basic_io.h	Implements batched cache-miss loading for MultiRead and shared in-flight page loads with stale prevention.
src/io/read_cache/read_cache_test.cpp	Adds CountingIO test backend and new tests for batched misses, concurrency sharing, and failure retry behavior.

Suppressed comments (1)

src/io/read_cache/read_cache_test.cpp:216

• This busy-wait loop spins without yielding, which can burn CPU during test runs. Adding a yield (or a condition_variable) makes the test friendlier and less noisy under contention.

        while (not start.load()) {
        }

[LHT129]

test review

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/io/common/basic_io.h:640

• ClearCache() sets LoadingPage::done = true and notifies waiters without ensuring state->page is populated. A concurrent LoadCachedPages() waiter can observe done==true, proceed, and read state->page == nullptr, causing spurious read failures.

ClearCache() should not set done/notify for in-flight loads; it should just clear the cache and mark in-flight loads as stale so FinishPageLoads() won’t insert them into the cache.

                for (const auto& [page_id, state] : loading_pages_) {
                    state->stale = true;
                    state->done = true;
                    states_to_notify.push_back(state);
                }

[LHT129]

Code review for 1791208.

[LHT129]

[suggestion] In InvalidateCacheRange, when the range overflows (offset > UINT64_MAX - (size - 1)), the function calls cache_->Clear() and returns early without marking in-flight LoadingPage entries as stale. This means an in-flight page load that completes after the clear could insert now-stale data into the empty cache via FinishPageLoads (since state->stale would still be false).

The normal path below (the for-loop over pages) correctly marks in-flight pages as stale via iter->second->stale = true. Consider also marking in-flight pages as stale in the overflow path for consistency:

if (offset > UINT64_MAX - (size - 1)) {
    cache_->Clear();
    for (const auto& [pid, state] : loading_pages_) {
        state->stale = true;
    }
    return;
}

This is a pre-existing pattern (the early return was there before this PR), but since this PR introduces the stale flag mechanism, the overflow path should be updated to use it.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[LHT129]

I reviewed the batched cache-miss changes across basic_io.h, page_cache.{h,cpp}, read_cache_test.cpp, and test_ivf.cpp. The implementation is well-structured and the test coverage is thorough.

Summary of changes:

• PageCache gains Acquire/Wait/Complete/IsStale for coordinating in-flight page loads, with a LoadingPage state tracked per page.
• BasicIO captures a ReadCacheSnapshot (shared_ptr + page_id_base) at the start of each read operation, providing snapshot isolation without holding cache_mutex_ across I/O.
• MultiRead now deduplicates pages across sub-reads, batches all cache misses into a single MultiReadImpl call, and copies results back from cached pages.
• GetOrLoadPage (single-page path) uses the same Acquire/Complete protocol, sharing in-flight loads with concurrent MultiRead callers.
• InvalidateCacheRange and ClearCache take a local snapshot of the cache pointer before operating, avoiding lock nesting with the page cache mutex.
• EnableReadCache now takes cache_mutex_ to prevent races with concurrent snapshot captures.
• Tests cover: batched misses, single-read backend for Read, shared in-flight loads, failure retry, snapshot isolation during cache replacement, and stale-page reload for both MultiRead and Read.

Observations (non-blocking):

1. GetOrLoadPage captures the ReadCacheSnapshot once and uses it across the entire retry loop. If SetReadCache replaces the cache between iterations, the function continues using the old snapshot (kept alive by shared_ptr). This is correct for snapshot isolation, but a page loaded into the old cache will not be visible to subsequent reads through the new cache — the next read for that page will incur another I/O. This is a reasonable tradeoff.

2. The LoadCachedPages retry loop calls pages.clear() before each retry, which is correct. The stale flag is properly propagated from LoadCachedPagesOnce through the bool& out-parameter.

3. The test helper CountingIO uses condition_variable-based blocking for deterministic thread synchronization, which is a significant improvement over the earlier timing-based approach. The BlockMultiRead/WaitForMultiReadBlock/UnblockMultiRead pattern is clean and race-free.

No blocking issues found. The PR looks ready to merge from a review perspective.

[LHT129]

Code Review for PR #2636

[critical] Deadlock in InitIO → EnableReadCache

In src/io/common/basic_io.h, InitIO calls GetReadCacheSnapshot() (which locks cache_mutex_ via std::scoped_lock) and then calls EnableReadCache() (which also locks cache_mutex_ via std::scoped_lock added in this PR). Since std::mutex is non-recursive, this causes a deadlock on the same thread.

The call chain:

1. InitIO → GetReadCacheSnapshot() acquires cache_mutex_ (line ~328)
2. InitIO → EnableReadCache() attempts to acquire cache_mutex_ again → deadlock (line ~395)

EnableReadCache added the scoped_lock in this PR, but InitIO was not updated to avoid the double-lock.

Suggested fix: restructure InitIO to release the snapshot before calling EnableReadCache:

inline void
InitIO(const IOParamPtr& io_param) {
    if constexpr (not InMemory) {
        bool should_enable = false;
        bool should_clear = false;
        {
            const auto cache = GetReadCacheSnapshot();
            if (cache.cache == nullptr) {
                should_enable = true;
            } else if (io_param != nullptr and io_param->enable_read_cache_) {
                should_enable = true;
            } else {
                should_clear = true;
            }
        }  // lock released here
        if (should_enable) {
            EnableReadCache(io_param);
        } else if (should_clear) {
            ClearCache();
        }
    }
    ...

Alternatively, remove the lock from EnableReadCache and document that callers must hold cache_mutex_.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Suppressed comments (3)

src/io/read_cache/read_cache_test.cpp:137

• This test-side wait has no timeout, so if the cache regresses and never enters MultiReadImpl, the whole test process hangs instead of reporting a failure. Use a bounded wait_for/wait_until and assert that the block was reached; the analogous read-block helper and start barrier should use the same protection.

    void
    WaitForMultiReadBlock() const {
        std::unique_lock<std::mutex> lock(multi_read_mutex_);
        multi_read_blocked_cv_.wait(lock, [this] { return multi_read_blocked_; });

src/io/read_cache/read_cache_test.cpp:86

• WriteImpl still writes the shared, non-atomic size_ on every overwrite. A concurrent cached read can access size_ while this helper writes it, so the invalidation tests retain a C++ data race (and this reintroduces the earlier test-helper issue). Only update size_ when the backing buffer grows, as the write below is not changing the logical extent.

        if (data_.size() < offset + size) {
            data_.resize(offset + size);
        }
        std::memcpy(data_.data() + offset, data, size);
        size_ = std::max(size_, offset + size);

src/io/common/basic_io.h:667

• This path allocates one full Page for every owned miss and then allocates a second contiguous read_data buffer containing all of those pages. The amount of temporary memory therefore scales with the number of unique misses, not max_pages_; a large MultiRead can cause a substantial memory spike or OOM even when the configured cache is small. Refill the misses in bounded chunks (or otherwise avoid retaining all page buffers until the whole request completes).

            if (success and not owned_loads.empty()) {
                if (batch_read) {
                    read_data.resize(total_size);

[LHT129]

Regarding the review note about a possible InitIO -> EnableReadCache self-deadlock: no mutex is retained by ReadCacheSnapshot. GetReadCacheSnapshot() acquires cache_mutex_ only in its local scoped_lock and releases it before returning the shared_ptr/page-id value object. InitIO therefore calls EnableReadCache() after that lock has already been released, so there is no recursive lock acquisition. I rechecked this path on ba35b29; no code change is needed for that note.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

src/io/common/basic_io.h:671

• This materializes every miss in the request as both a full 128 KiB Page and a packed read_data buffer before invoking the backend. A sparse or large MultiRead can therefore allocate far more than the cache's max_pages_ bound (for example, 10,000 misses require roughly 2.4 GiB for these two temporary buffers), causing avoidable memory spikes or OOM. Load owned misses in bounded chunks, or otherwise cap and release the temporary storage.

        std::vector<PagePtr, AllocatorWrapper<PagePtr>> loaded_pages(allocator_);
        std::vector<uint64_t, AllocatorWrapper<uint64_t>> read_sizes(allocator_);
        std::vector<uint64_t, AllocatorWrapper<uint64_t>> read_offsets(allocator_);
        std::vector<uint8_t, AllocatorWrapper<uint8_t>> read_data(allocator_);

[LHT129]

Addressed the three suppressed Copilot observations in 7dd1bed:

• Test synchronization now uses bounded 5-second waits for the multi-read/read block helpers and the reader start barrier. Failure paths still unblock and join worker threads, so regressions report assertions rather than hanging the test process.
• CountingIO::WriteImpl updates size_ only when the backing buffer grows; same-extent overwrite/invalidation tests no longer race with readers on size_.
• I reviewed the temporary-memory observation. max_pages limits resident cache entries, not pages retained by an active MultiRead result. This path intentionally issues one backend MultiReadImpl call for all unique misses to minimize remote-IO round trips; splitting by max_pages would weaken the batching objective and still requires the caller-visible pages to remain alive. The existing allocation/overflow failures are handled without publishing partial loads. A scatter-gather backend would be the appropriate future way to remove the contiguous read_data copy without sacrificing one-call batching.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/io/read_cache/page_cache.cpp:163

• Please add a regression test for calling Clear() while a page load is in flight, verifying that the waiter wakes with a stale result and a later acquire can retry. The new done/stale transition here is not covered by the existing tests (which cover Remove() and insertion failures), and a missed notification would leave readers blocked indefinitely.

        for (const auto& [page_id, state] : loading_pages_) {
            state->stale = true;
            state->done = true;
            states_to_notify.push_back(state);
        }

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/io/common/basic_io.h:708

• When this path is reached because stale is true, the batch will retry, but abandon_owned_loads() completes every owned handle as a non-stale failure. A concurrent waiter on one of those otherwise-valid pages then receives nullptr, sees IsStale == false in GetOrLoadPage, and returns a read failure instead of retrying. Cancel the owned loads as stale (or otherwise signal retry) when aborting a stale batch, while preserving the non-stale failure behavior for genuine load failures.

                    auto page = cache.cache->Wait(result.handle);
                    stale = stale or cache.cache->IsStale(result.handle);
                    if (page == nullptr or stale) {
                        abandon_owned_loads();
                        return false;

src/io/common/basic_io.h:372

• This mutex is now taken on every Release call, including InMemory BasicIO instantiations where the cache branch is compile-time disabled and cached_direct_reads_ can never contain an entry. That adds unnecessary contention to allocated direct reads (for example, cross-block MemoryBlockIO reads); guard this lookup with if constexpr (not InMemory) to preserve the lock-free in-memory path.

        {
            std::scoped_lock<std::mutex> lock(cached_direct_reads_mutex_);
            if (cached_direct_reads_.erase(data) != 0) {

[LHT129]

[suggestion] The Release method lost its if constexpr (not InMemory) guard. For InMemory IO types, this now unconditionally acquires cached_direct_reads_mutex_ on every Release call even though cached_direct_reads_ is always empty for those types.

The original code guarded the entire cache-related block with if constexpr (not InMemory), which avoided the lock overhead for in-memory IOs. Consider restoring that guard around the cached_direct_reads_ lookup.

[LHT129]

[suggestion] GetReadCacheSnapshot() returns a snapshot by value under cache_mutex_, but the returned shared_ptr<PageCache> is then used without the mutex. While this is safe (shared_ptr reference counting is thread-safe, and PageCache has its own internal mutex), the snapshot captures cache_page_id_base_ at a single point in time. If EnableReadCache is called concurrently, the snapshot may refer to a stale cache that is no longer the active one. This is intentional and correct (in-flight operations should complete against the cache they started with), but worth documenting as a design decision.

[LHT129]

[note] The cached_direct_reads_ set tracks heap pointers returned by the cached direct-read path so Release() can route deallocation to the allocator instead of the IO backend. The set uses UnorderedSet<const uint8_t*> keyed by raw pointer value. If the allocator ever reuses a freed address before the corresponding Release() call, a stale entry could cause a double-free or use-after-free. In practice this requires a missed Release() call (a caller bug), but worth noting the implicit contract: every pointer returned via need_release=true must be paired with exactly one Release() call.

[LHT129]

[suggestion] Complete() catches exceptions from InsertLocked() and stores them in error, then sets state->done = true and notifies waiters before rethrowing. This ensures waiters are never left blocked indefinitely even when eviction fails. However, the waiter receives state->page = nullptr (since result stays null when InsertLocked throws). The caller in LoadCachedPagesOnce treats a null page from Complete as a failure and will retry the entire batch if stale is true. This is correct behavior: the eviction failure is transient and retrying may succeed after other pages are released.

One edge case: if InsertLocked throws std::bad_alloc, retrying is unlikely to help. Consider whether eviction failures should be distinguished from allocation failures for retry decisions.

[LHT129]

Review Summary

This PR has been through multiple review iterations and the code quality is now solid. The core changes are well-designed:

Strengths:

• The Acquire/Wait/Complete/IsStale API on PageCache correctly implements in-flight page load sharing, preventing duplicate I/O for concurrent readers requesting the same page.
• The ReadCacheSnapshot pattern cleanly decouples read operations from cache lifecycle changes — readers hold a consistent view of the cache they started with.
• MultiRead now deduplicates page requests and batches cache misses into a single MultiReadImpl backend call, which is the primary performance goal of this PR.
• Stale page detection and retry logic is correctly wired through the entire stack: PageCache::Remove marks in-flight loads stale → Complete skips insertion for stale pages → LoadCachedPages retries the batch.
• Exception safety is thorough: abandon_owned_loads ensures in-flight loads are completed (as failures) on any error path, preventing waiter threads from blocking indefinitely.
• Test coverage is comprehensive: batched misses, cross-page reads, concurrent load sharing, cache snapshot during in-flight reads, invalidation + reload, failed-load retry, and eviction-during-insert.

Minor suggestions posted inline:

1. ReadCacheSnapshot design is correct but the snapshot semantics are implicit — consider a brief comment.
2. cached_direct_reads_ raw-pointer set has an implicit contract that every need_release=true pointer must be paired with exactly one Release().
3. Complete() retry-on-eviction-failure could potentially loop on std::bad_alloc — consider distinguishing transient eviction failures from fatal allocation failures.

No blocking issues found. The PR is ready to merge from a code review perspective.