Skip to content

perf(pm): dedup version-manifest disk writes via OnceMap - #2882

Closed
elrrrrrrr wants to merge 1 commit into
nextfrom
perf/dedup-version-manifest-disk-write
Closed

elrrrrrrr wants to merge 1 commit into
nextfrom
perf/dedup-version-manifest-disk-write

Conversation

@elrrrrrrr

Copy link
Copy Markdown
Contributor

Problem

DiskManifestStore::store_version_manifest is called from service/registry.rs:388 inside the inflight_version<(name, original_spec)> gate. The gate keys on the original spec, but the disk write keys on the resolved version. Two sibling specs (^4.17.0, ~4.17.20) resolving to the same 4.17.21 each pass through different gates and both spawn write_json for the same path:

spec="^4.17.0"  ──┬─ inflight gate 1 ─→ resolved 4.17.21 ─→ tokio::spawn write 4.17.21.json
spec="~4.17.20" ──┴─ inflight gate 2 ─→ resolved 4.17.21 ─→ tokio::spawn write 4.17.21.json
spec="4.x"      ────  inflight gate 3 ─→ resolved 4.17.21 ─→ tokio::spawn write 4.17.21.json

Each duplicate does:

  • redundant serde_json::to_vec (~5 KB × N)
  • redundant disk write
  • truncate-race window where a concurrent reader sees partial JSON (read fails → cache miss)

store_versions (versions.json) is safe — it sits inside inflight_full<name> whose key matches the write key.

Fix

Add an OnceMap<(name, resolved_version), ()> to DiskManifestStore. Reuse the existing OnceMap::register (sync, atomic "first wins") that ruborist already uses for inflight_full / inflight_version. Subsequent callers for the same (name, version) get None from register and skip the spawn entirely.

let Some(notify) = self.inflight_version_writes.register(key.clone()) else {
    return;  // duplicate, skip
};
let inflight = Arc::clone(&self.inflight_version_writes);
tokio::spawn(async move {
    write_json(&path, &*manifest).await;
    inflight.complete(key, Some(()), notify);
});

Impact

ant-design preload: ~600 specs → ~250 unique versions → ~350 redundant spawn + serialize + write cycles eliminated, ~1.75 MB redundant disk write avoided. Wall-clock impact within σ (writes are fire-and-forget off the resolve hot path), but reduces CPU/IO contention with preload workers and closes the truncate-race window.

Test plan

  • cargo build --profile release-local -p utoo-pm clean
  • cargo clippy -p utoo-pm -p utoo-ruborist --all-targets -- -D warnings --no-deps clean
  • cargo test -p utoo-pm — 245 passed, 0 failed
  • CI `bench-phases-linux` — confirm no regression

🤖 Generated with Claude Code

The upstream `inflight_version<(name, original_spec)>` gate keys on the
*original* spec while `store_version_manifest` keys on the *resolved*
version. Two sibling specs (`^4.17.0`, `~4.17.20`) resolving to the same
`4.17.21` each pass through different gates and would each
`tokio::spawn(write_json)` for the same path — duplicate serialize,
duplicate disk write, and a brief truncate-race window where a
concurrent reader sees partial JSON.

Add an `OnceMap<(name, version), ()>` to `DiskManifestStore` and use
`register` as a sync atomic "first wins" check. Subsequent callers for
the same `(name, resolved_version)` skip the spawn entirely.

For ant-design preload (~600 specs → ~250 unique versions), this drops
~350 redundant spawn+serialize+write cycles (~1.75 MB redundant disk
write).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@elrrrrrrr
elrrrrrrr marked this pull request as draft May 1, 2026 00:56

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces a deduplication mechanism for disk writes in DiskManifestStore using a OnceMap to prevent redundant concurrent tasks for the same version manifest. Feedback was provided regarding the error handling in the asynchronous write task; specifically, the operation is marked as complete even if the write fails, which would prevent subsequent retry attempts for that manifest. It is recommended to only mark the operation as complete upon a successful write.

Comment on lines +93 to +94
write_json(&path, &*manifest).await;
inflight.complete(key, Some(()), notify);

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.

medium

The write is currently marked as complete with Some(()) regardless of whether write_json succeeds or fails. Since OnceMap::register returns None for any key already in the Done state, a single failed write (e.g., due to a transient I/O error or disk full condition) will prevent any subsequent attempts to persist that specific version manifest for the lifetime of the process. This results in persistent cache misses for that version.

Consider updating write_json to return a success boolean and using inflight.complete(key, success.then_some(()), notify) here. This ensures that if a write fails, the key is removed from the map (via None), allowing future calls to retry the operation.

@elrrrrrrr

Copy link
Copy Markdown
Contributor Author

Superseded by #2874 commit f96c6e82 (perf(ruborist): dedup store_version_manifest disk writes per resolved version), which is identical in scope: DashSet<(name, resolved_version)> gating around the disk write. Closing to consolidate.

@elrrrrrrr elrrrrrrr closed this May 1, 2026
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.

1 participant