#1381

perf: skip redundant disk writes on get_or_fetch memory hits with WriteOnInsertion

Openzach-schoenbergerwants to merge intofrom

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:

SourceEnqueue?Behavior vs. main
Outeryesunchanged
Diskyesunchanged: 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)
Memoryonly 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 (Outer for a source fetch), so N concurrent callers still enqueue N times, as on main. #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 --tests passes, including the two foyer-opendal refill tests above.
  • cargo fmt -p foyer --check is clean, and cargo clippy -p foyer --all-targets reports no new warnings.
  • No test covers the block engine's Old-entry rewrite through get_or_fetch (storage_block_engine_enqueue_skip / Age::Old), so keeping Source::Disk enqueued 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.