Skip to content

[core] Reduce shared disk cache lock contention - #1186

Merged
JingsongLi merged 6 commits into
apache:mainfrom
XiaoHongbo-Hope:codex/shared-cache-reduce-contention
Oct 21, 2026
Merged

JingsongLi merged 6 commits into
apache:mainfrom
XiaoHongbo-Hope:codex/shared-cache-reduce-contention

Conversation

@XiaoHongbo-Hope

@XiaoHongbo-Hope XiaoHongbo-Hope commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Purpose

Follow-up to #1175: reduce contention and foreground publication work in the opt-in node-shared disk cache under multi-process range reads.

Brief change log

  • Stripe download coordination by the full block key instead of serializing every block of one file. Same-block requests still coordinate; whole-file/range overlap can duplicate downloads.
  • Read immutable cached inodes without the directory lock, checking the mutation marker and generation before and after the read. Throttle recency writes to approximately once every ten seconds.
  • Validate the owned encoded buffer and return a Bytes slice instead of copying its payload into another allocation.
  • Return range data before publication when a bounded background-fill slot is available. Transfer download guards into the fill task; use synchronous backpressure at saturation or for blocks larger than 1 MiB. Whole-file fills stay synchronous.
  • Encode/write a locked temporary inode outside the directory gate. Keep generation validation, eviction, capacity accounting and atomic rename under the gate. Recovery skips live staging files and cleans abandoned ones after process exit.
  • Document transient staging space and the scope of the concurrency bounds.

No experimental timer module or environment-controlled retry tuning is included.

Tests

The experimental candidate passed all 81 cache tests and ten repeated complete cache-test runs. The cleaned PR branch also passed all 81 cache tests, formatting and whitespace checks with:

cargo +1.94.0 test --release --locked -p paimon io::cache --lib -j4
cargo +1.94.0 fmt --all -- --check
git diff --check

Coverage includes owned-buffer CRC/key/truncation validation, block-striped download coordination, hits during directory-lock contention, generation invalidation, bounded asynchronous publication/single-flight, live staging during recovery, process-exit orphan cleanup, and capacity/priority accounting.

The broader io:: test filter also passed all 184 tests with proxy variables removed for localhost HTTP fixtures. The first run with inherited proxy settings had one HTTP error-message assertion failure (183 passed); the exact test and full filter passed after unsetting HTTP(S)/ALL_PROXY and setting NO_PROXY to localhost/127.0.0.1. No source change was needed for that retest.

Performance evidence and limitations

A complete training DataLoader experiment used one 64-vCPU/256-GiB host, a 60-CPU/200-GiB process scope, 8 ranks x 4 workers, batch 16/rank, prefetch 2, 64 warmup + 128 measured batches, per-batch synchronization, fixed input snapshots and sequences, a fresh tmpfs cache per trial, 20-GB configured capacity and 1-MB blocks. The pipeline included Steps, pose, images and JPEG decoding; image bodies used the existing Python OSS path, not Rust BLOB reading.

The staging change alone, on top of the previous experimental lockless-hit/asynchronous-fill candidate, changed the two-run median from 225.2 to 294.7 samples/s (+30.8%). This is not a direct main-versus-PR benchmark. Peak memory did not improve (93.3 to 94.4 GiB), and node RX/sample increased slightly (4.66 to 4.77 MiB). A single cache-disabled reference reached 367.7 samples/s and 7.57 MiB/sample. Thus caching still traded throughput and memory for lower network reception in this workload. No claim of a disk-cache result or universal speedup is made.

Diagnostic instrumentation was disabled for those headline trials. The cleaned PR branch removes that instrumentation and is not separately full-DataLoader benchmarked. Smoke value hashes matched and full-run sample sequences/image counts matched; full-run output value hashes were not enabled. Remaining directory-locked eviction and download coordination are intentionally not redesigned here.

API and Format

No public API, configuration default, or published cache-block format change. Only shared mode uses background fills. Eight fill slots and eight staging slots are per cache instance, not per node; temporary staging bytes are additional to the published capacity. Published blocks remain immutable, and storage mutation fencing remains mandatory.

Documentation

Updated the shared-cache section of docs/src/getting-started.md with concurrency, staging headroom, and whole-file/range overlap behavior.

@XiaoHongbo-Hope
XiaoHongbo-Hope marked this pull request as ready for review October 21, 2026 08:20
@JingsongLi

Copy link
Copy Markdown
Contributor

[P2] Avoid retaining the block lock on an idle caller runtime

In reader.rs:115–120, the detached fill owns load_guard after the foreground read has returned. With a Tokio current-thread runtime, block_on returning stops driving that task. If runtime A remains alive but idle, a subsequent read of the same block through the same cache on runtime B waits indefinitely at load_lock.lock_owned().await. The shared file lock's two-second timeout cannot help, because the process-local mutex is acquired first. This introduces a runtime-lifetime dependency after a completed read; the previous synchronous publication released the guard before returning.

Reproduction

  1. Add the following test inside the existing tests module in crates/paimon/src/io/cache/reader.rs. It uses that module's existing CountingReader and imports. The small block size keeps the example minimal; shared mode's default 1 MiB blocks also select this background-fill path. This unit test constructs CachedFileReader directly; when reproducing through FileIO, use an eligible metadata path or include data in local-cache.whitelist.
#[test]
fn review_1186_cross_runtime_read_does_not_stall_behind_idle_fill() {
    let directory = tempfile::tempdir().unwrap();
    let delegate = Arc::new(CountingReader {
        data: Bytes::from_static(b"abcdefgh"),
        reads: AtomicUsize::new(0),
    });
    let cache = Arc::new(
        LocalCache::new(LocalCacheConfig {
            shared: true,
            dir: Some(directory.path().to_path_buf()),
            namespace: "test".into(),
            max_size: Some(4096),
            block_size: 4,
            whitelist: std::collections::HashSet::from([FileType::Meta]),
        })
        .unwrap(),
    );
    let reader = CachedFileReader::new(delegate.clone(), "data.parquet", 8, cache);
    let first_runtime = tokio::runtime::Builder::new_current_thread()
        .enable_all()
        .build()
        .unwrap();
    assert_eq!(first_runtime.block_on(reader.read(0..4)).unwrap(), "abcd");

    // Keep the first runtime alive, but stop driving it.
    let second_runtime = tokio::runtime::Builder::new_current_thread()
        .enable_all()
        .build()
        .unwrap();
    let result = second_runtime.block_on(async {
        tokio::time::timeout(std::time::Duration::from_millis(500), reader.read(0..4)).await
    });
    assert!(
        result.is_ok(),
        "a completed read must not hold a block lock on its idle caller runtime"
    );
    assert_eq!(result.unwrap().unwrap(), "abcd");
}
  1. Run:
cargo +1.94.0 test --locked -p paimon --lib \
  --no-default-features --features storage-fs,storage-memory -j4 \
  review_1186_cross_runtime_read_does_not_stall_behind_idle_fill

Expected: both reads finish successfully. Actual on 0bcca964: the second read hits the 500 ms test timeout; without the timeout it remains blocked while runtime A stays alive and idle. No artificial directory-lock contention is required.

Verified on macOS with Rust 1.94.0: the identical test passes on base e4ec3a45 and fails on head 0bcca964. The full io::cache filter on that head passes all 81 existing tests, with only this added regression test failing.

Suggested fix

A minimal fix is to keep publication synchronous when the caller uses RuntimeFlavor::CurrentThread. Alternatively, schedule fills on a cache-owned executor that is guaranteed to keep running independently of the caller's block_on. Preserve same-block single-flight coordination, but ensure that a completed foreground read cannot leave later reads dependent on an idle caller runtime. Please retain a cross-runtime regression test for this case.

@JingsongLi JingsongLi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@JingsongLi
JingsongLi merged commit 8d08e48 into apache:main Oct 21, 2026
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants