Goal
With HybridCachePolicy::WriteOnInsertion, every get_or_fetch that is served from the in-memory cache re-enqueues the entry for the disk cache, even when the disk cache already holds it. A frequently read key therefore produces a disk write on every get_or_fetch call, even though nothing changed.
This PR stops those redundant writes while keeping the case where re-enqueueing a memory hit is needed.
Background
Before #1177, the enqueue in the fetch future was gated on the fetch's diversion context (this.inner.store().is_some()). That context was only set when the entry came from the disk-load or outer-fetch path, so memory hits and waiting callers never enqueued:
if entry.properties().location() != Location::InMem
&& *this.policy == HybridCachePolicy::WriteOnInsertion
&& this.inner.store().is_some()
{
let throttled = this.inner.store().as_ref().unwrap().throttled;
if !throttled {
this.storage.enqueue(entry.piece(), false);
}
}#1177 replaced that check with this.store.is_enabled(), which is true for every hybrid cache with a disk cache. From then on every successful get_or_fetch enqueued, memory hits included. I could not find a stated reason for this in #1177, #1186 or #1187; it reads as a side effect of the refactor. #1187 later added Source::{Outer, Memory, Disk} but did not use it here.
Re-enqueueing a memory hit is still sometimes needed. Under WriteOnInsertion the memory cache does not pipe evictions to disk, so an entry can be in memory but missing from disk: its disk write failed, the disk engine evicted it, or the admission filter rejected it. A later memory hit is the only thing that puts it back. foyer-opendal's engine_tests::failed_write_releases_and_retry_or_close_stays_bounded and engine_tests::fifo_eviction_allows_young_entry_refill cover this; both fail if memory hits are skipped outright.
Change
HybridGetOrFetch::poll (foyer/src/hybrid/cache.rs) now also checks the entry's Source before enqueueing:
Source | Enqueue? | Behavior vs. main |
|---|---|---|
Outer | yes | unchanged |
Disk | yes | unchanged: the engine still sees disk hits with their Age, so BlockEngine keeps skipping Young entries and rewriting Old ones before their block is evicted (#987) |
Memory | only if !store.may_contains(key) | changed: memory hits for keys already on disk, or already queued in the keeper (#1337), are skipped; missing entries are still refilled |
The existing conditions (location not InMem, WriteOnInsertion, store enabled, not throttled) and the hit/miss metrics are unchanged.
Store::may_contains can return a false positive on a hash collision. That only skips a refill write; it never serves wrong data, and a later miss still reaches the fetch closure.
Not in scope
- Duplicate writes from concurrent callers of one fetch. A caller waiting on another caller's fetch receives the entry with that fetch's
Source(Outerfor a source fetch), so N concurrent callers still enqueue N times, as onmain. #1351 tracks write coalescing for this in the engine, and the OpenDAL engine already merges same-key pending writes (#1360). This PR covers the repeated memory-hit case at the hybrid-cache layer, which applies to every engine. WriteOnEviction. Unaffected. #1346 separately handles disk hits that are written back through the eviction pipe.
Related: #1351, #1346, #1177, #987.
Testing
cargo test --workspace --lib --testspasses, including the twofoyer-opendalrefill tests above.cargo fmt -p foyer --checkis clean, andcargo clippy -p foyer --all-targetsreports no new warnings.- No test covers the block engine's
Old-entry rewrite throughget_or_fetch(storage_block_engine_enqueue_skip/Age::Old), so keepingSource::Diskenqueued is based on reading the code (#987,BlockEngine::enqueue), not on a test. I can add a regression test for it if reviewers want one.