feat(io): add optional read cache for disk IO

[LHT129]

Summary

Add an optional page-based read cache to concrete disk IO implementations.

Closes #2420

Configuration

Keep the concrete IO type and set enable_read_cache to true. total_cache_size supplies the page-cache budget.

Validation

• make fmt
• git diff --check
• source ~/.bashrc && cmake -S . -B build-release && cmake --build build-release --target datacell -j 1

Local test configuration is blocked by the environment Boost mirror HTTP 403; CI runs the full test suite.

[vsag-bot]

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

[vsag-bot]

Automated pull request review completed.

Review effort: high (1501 changed lines across 41 files).

Submitted 3 inline comments.
Review: #2567 (review)

[mergify]

Merge Protections

🟢 All 3 merge protections satisfied — ready to merge.

Show 3 satisfied protections

🟢 Require kind label

• label~=^kind/

🟢 Require version label

• label~=^version/

🟢 Require linked issue for feature/bug PRs

• body~=(?im)(?:^|[\s\-\*])(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s*:?\s+(?:#\d+|[\w.\-]+/[\w.\-]+#\d+|https?://github\.com/[\w.\-]+/[\w.\-]+/issues/\d+)

[LHT129]

Code Review Summary

Thanks for this PR! The page-based read cache is a well-structured addition. Here are my findings:

Already Noted (by self-review)

• Page constructor null check — Allocate return value not checked (page.h)
• GetOrLoadPage mutex contention — cache mutex held during disk I/O (basic_io.h)
• PageCache::Insert eviction logic — fallback to pages_.begin()->first() (page_cache.cpp)

Additional Findings

[suggestion] GetMemoryUsage doesn't account for cache memory
The BasicIO::GetMemoryUsage() method only returns size_, not including the page cache memory. When the read cache is enabled, the actual memory footprint can be significantly larger, leading to inaccurate memory budgeting. Consider adding cache_->Size() * Page::DEFAULT_PAGE_SIZE to the returned value.

[suggestion] Serialize populates cache with transient reads
Serialize calls this->Read(...) which goes through ReadCached when the cache is enabled. This populates the cache with every page read during serialization, potentially evicting useful pages that were already cached. Consider clearing the cache before serialization or adding a bypass flag.

[note] Shrink return removal
The Shrink method previously had return cast().ShrinkImpl(size); which prevented the else branch from executing. Now the return is removed so ClearCache() always executes. This is correct behavior, but worth confirming no ShrinkImpl implementation relied on the old early-return.

[suggestion] DirectReadImpl always sets need_release=true with cache
When the cache is active, the DirectReadImpl overload that returns const uint8_t* always allocates and copies, setting need_release = true. This changes semantics for callers that previously relied on zero-copy reads. Worth documenting.

[note] BucketDataCell GetMemoryUsage under-reports
AdjustBucketReadCacheParam creates separate IO instances per bucket, each with its own cache, but BucketDataCell::GetMemoryUsage() doesn't include cache memory. Consistent with existing IOArray behavior, but worth noting for capacity planning.

Overall Assessment

The design is clean: pluggable eviction policy (PageCache base + LRUPageCache), shared_ptr ownership so evicted pages remain valid for in-flight readers, and proper cache invalidation on writes. The test coverage is good with unit tests for PageCache/LRUPageCache and integration tests for the full read/write/invalidation flow. The examples are helpful for demonstrating usage.

No blocking issues found. The suggestions above are non-blocking improvements.

[vsag-bot]

Automated inline review completed.

Review effort: high (1407 changed lines across 39 files).
Submitted 2 inline comments.

Reviewed commit 44c70ef.

[vsag-bot]

Automated inline review completed.

Review effort: high (1420 changed lines across 39 files).
Submitted 4 inline comments.

Reviewed commit a5f9bb5.

[vsag-bot]

Automated inline review completed.

Review effort: high (1452 changed lines across 39 files).
Submitted 4 inline comments.

Reviewed commit ed0f8f0.

[LHT129]

Code review for PR #2567: Add optional read cache for disk IO.

Summary: This PR introduces a page-based read cache (128KB pages) with LRU eviction for disk IO backends. The cache integrates into the CRTP-based BasicIO<IOTmpl> template and is enabled via IO parameters. IVF bucket datacells distribute the cache budget evenly across buckets.

Overall Assessment: The design is clean and well-integrated into the existing IO abstraction. The cache correctly handles write-through invalidation, resize/shrink/deserialize cache clearing, and cross-page boundary reads. The test coverage is reasonable for unit-level testing of the cache components.

General Notes:

1. [note] DirectReadImpl bypass with cache: When the read cache is enabled, DirectReadImpl is bypassed entirely — the cache path allocates a new buffer, copies data into it via ReadCached, and the caller must call Release to free it. This changes the semantics from zero-copy (returning an internal pointer) to always-copy when the cache is active. Callers that rely on need_release == false for performance (e.g., scan_bucket_by_id in bucket_datacell.h) will now always pay the allocation + copy cost. This is expected given the cache architecture, but worth being aware of for hot paths.

2. [suggestion] GetMemoryUsage does not include cache: The GetMemoryUsage() method only returns size_, which does not account for the memory consumed by the page cache. When the read cache is enabled, the actual memory footprint can be significantly larger, leading to inaccurate memory budgeting. Consider including cache memory: usage += cache_->Size() * Page::DEFAULT_PAGE_SIZE.

3. [suggestion] DirectReadImpl overload documentation: The DirectReadImpl overload (returning const uint8_t* with need_release) allocates a temporary buffer and copies through ReadCached. When the cache is active, this always sets need_release = true. It would be helpful to document this behavioral change in the method comment.

4. [note] NonContinuousIO now routes through BasicIO: The change from inner_io_->WriteImpl(...) to inner_io_->Write(...) (and similarly MultiReadImpl → MultiRead) correctly routes through BasicIO's public interface which handles cache invalidation and cache reads. This is a necessary change for read cache correctness.

5. [note] Shrink now always clears cache: The Shrink method previously had return cast().ShrinkImpl(size); which prevented the else branch from executing. Now ClearCache() always executes after Shrink. This is correct behavior, but worth confirming no ShrinkImpl relies on the old early-return behavior.

See inline comments for specific findings.

[vsag-bot]

Automated inline review completed.

Review effort: high (1460 changed lines across 40 files).
Submitted 3 inline comments.

Reviewed commit 5b2e73d.

[vsag-bot]

Automated inline review completed.

Review effort: high (1500 changed lines across 41 files).
Submitted 1 inline comment.

Reviewed commit 03199f7.

[LHT129]

Code review for commit 4d926ac.

[vsag-bot]

Automated inline review completed.

Review effort: high (1501 changed lines across 41 files).
Submitted 3 inline comments.

Reviewed commit 4d926ac.

[wxyucs]

lgtm

[LHT129]

[P2] RebuildSupplementIOParam does not halve the cache size in hybrid mode\n\nWhen supplement_io_type_ is non-empty (hybrid IO — one-bit and supplement use different backends), this path clones the JSON via ToJson() without halving read_cache_total_size_. In contrast, when the two storages share the same IO type, the if branch above calls SuffixIOParam(io_param, "_supplement", true) which correctly halves the cache.\n\nIn hybrid mode, both the one-bit cell (via SuffixIOParam with split_cache=false) and the supplement cell (via this path without halving) each receive the full configured cache size, consuming 2× the expected memory. This is consistent with the constructor path (where supplement_io_param is passed directly), but it means the user-facing precise_cache_total_size parameter does not represent the total cache budget for the precise component — it is per-IO-object.\n\nConsider either:\n1. Halving the cache here when enable_read_cache_ is true (matching the shared-IO behavior), or\n2. Documenting that in hybrid mode the cache size applies per-IO-backend, not per-component.

[LHT129]

[note] EnableReadCache(param) is called here but MemoryIO declares static constexpr bool InMemory = true, so the if constexpr (not InMemory) guard inside EnableReadCache makes this a compile-time no-op. The same pattern exists in MemoryBlockIO.\n\nThis is harmless but may confuse readers into thinking these in-memory IO types support read caching. Consider removing these calls or adding a comment explaining they are intentionally no-ops for consistency with the other IO constructors.

[LHT129]

[note] The per-bucket cache size calculation pages_per_bucket * Page::DEFAULT_PAGE_SIZE truncates due to integer division. For example, with total_cache_size = 10 * 128KB and bucket_count = 3, each bucket gets 3 * 128KB = 384KB, totaling 3 * 384KB = 1152KB across all buckets, losing 128KB (1 page) of the original 1280KB cache budget.\n\nThis is minor (at most (bucket_count - 1) * Page::DEFAULT_PAGE_SIZE bytes lost), but worth being aware of when configuring small caches with many buckets.

[LHT129]

[note] io_param->read_cache_total_size_ / 2 uses integer division, which truncates when the cache size is odd. For example, if read_cache_total_size_ = 1, each half gets 0, effectively disabling the cache for both halves. While the default cache size (256MB) is always even, user-configured values could be odd.\n\nThis is extremely minor (at most 1 byte lost), but consistent with the same pattern in AdjustBucketReadCacheParam.

[LHT129]

[note] Dead EnableReadCache calls for InMemory IOs

MemoryIO and MemoryBlockIO both call EnableReadCache(param) in their IOParamPtr constructors, but BasicIO<IOTmpl>::EnableReadCache is a no-op when InMemory is true (guarded by if constexpr (not InMemory)).

These calls are dead code — they compile to nothing and serve no purpose. Consider removing them to avoid misleading future readers into thinking InMemory IOs support caching.

[LHT129]

[note] AdjustBucketReadCacheParam rounding loss

uint64_t pages_per_bucket = total_pages / bucket_count;
json[...].SetUint64(pages_per_bucket * Page::DEFAULT_PAGE_SIZE);

Integer division truncates, so up to (bucket_count - 1) * 128KB of the cache budget is silently discarded. For example, with 10 buckets and 256MB total (2048 pages), each bucket gets 204 pages (204 * 10 = 2040), losing 8 pages (1MB).

Consider distributing the remainder across buckets (e.g., first N buckets get one extra page) or using total_cache_size / bucket_count directly on the byte value.

[LHT129]

[note] SuffixIOParam split_cache integer division rounding

json[...].SetUint64(io_param->read_cache_total_size_ / 2);

When read_cache_total_size_ is odd, integer division truncates 1 byte. While negligible in practice (1 byte out of 256MB default), this is the same pattern as AdjustBucketReadCacheParam and could be made consistent by using page-aligned division:

uint64_t half_pages = (io_param->read_cache_total_size_ / Page::DEFAULT_PAGE_SIZE) / 2;
json[...].SetUint64(half_pages * Page::DEFAULT_PAGE_SIZE);

[LHT129]

[P2] RebuildSupplementIOParam does not halve cache in hybrid mode

When supplement_io_type_ is non-empty (hybrid IO mode), RebuildSupplementIOParam builds a custom JSON parameter but never applies the split_cache halving logic. This means in hybrid mode both the one-bit and supplement cells each get the full cache size — 2× the expected memory budget.

The constructor path (line 186) correctly passes split_cache=true for the supplement cell, but RebuildSupplementIOParam (used during InitIO) only appends the suffix path and IO type override. The read_cache_total_size_ from the original io_param flows through unchanged.

Suggested fix: add the same split_cache logic inside the !supplement_io_type_.empty() branch:

if (io_param->enable_read_cache_) {
    json[READ_CACHE_TOTAL_CACHE_SIZE_KEY].SetUint64(io_param->read_cache_total_size_ / 2);
}

[LHT129]

[P2] RebuildSupplementIOParam does not halve cache in hybrid mode

In src/datacell/rabitq_split_datacell.h, RebuildSupplementIOParam (line 1002-1015) builds a custom JSON parameter when supplement_io_type_ is non-empty (hybrid IO mode), but never applies the split_cache halving logic. This means in hybrid mode both the one-bit and supplement cells each get the full cache size — 2× the expected memory budget.

The constructor path (line 186) correctly passes split_cache=true for the supplement cell, but RebuildSupplementIOParam (used during InitIO) only appends the suffix path and IO type override. The read_cache_total_size_ from the original io_param flows through unchanged.

Suggested fix: add the same split_cache logic inside the !supplement_io_type_.empty() branch:

if (io_param->enable_read_cache_) {
    json[READ_CACHE_TOTAL_CACHE_SIZE_KEY].SetUint64(io_param->read_cache_total_size_ / 2);
}

[vsag-bot]

Automated inline review completed.

Review effort: high (1501 changed lines across 41 files).
Submitted 3 inline comments.

Reviewed commit 937ad55.

[vsag-bot]

[P2] Do not hold the cache-wide lock during disk reads

This mutex remains held through ReadImpl, so one cache miss blocks every hit and miss for unrelated pages until its disk IO completes. Concurrent HGraph/IVF queries can therefore collapse to single-reader throughput whenever the working set misses the cache. Load outside this global lock and synchronize only lookup/insertion or individual page loads.

[vsag-bot]

[P2] Account for resident cache pages in memory usage

This cache can retain page_count * 128 KiB, but BasicIO::GetMemoryUsage() does not include it and disk-backed datacells omit their IO memory. Consequently a warmed default 256 MiB cache is invisible to HGraph/IVF GetMemoryUsage(), contradicting that API's current-footprint contract and breaking memory budgeting. Include resident pages and metadata in the reported totals.

[vsag-bot]

[P3] Accept nonnegative signed integer cache sizes

JsonWrapper::SetInt(131072) stores a signed integer, so this check rejects a valid nonnegative cache size even though GetUint64() safely handles it. Programmatically constructed parameters therefore behave differently from equivalent parsed JSON. Accept integer values of either representation and reject only negative signed values.