gix-ref: fast-path packed-only ref resolution in transactions#2705
gix-ref: fast-path packed-only ref resolution in transactions#2705Nik B (nikicat) wants to merge 3 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c445bda1c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| if !loose.contains(name) { | ||
| return packed.try_find(name).map(|opt| opt.map(Into::into)).map_err(Error::from); |
There was a problem hiding this comment.
When a ref is absent from the precomputed loose_names, this branch returns the packed value without checking the loose file after the per-ref lock is acquired. If another process creates or updates the loose shadow for a packed ref after the snapshot but before this transaction obtains that ref's lock, the CAS check is performed against the stale packed value and can overwrite the concurrent loose update; the previous ref_contents() read happened after locking and avoided this race.
Useful? React with 👍 / 👎.
| let loose_names: Option<std::collections::HashSet<FullName>> = packed_buffer | ||
| .and(store.loose_iter().ok()) | ||
| .map(|iter| iter.filter_map(Result::ok).map(|r| r.name).collect()); |
There was a problem hiding this comment.
store.loose_iter() only walks the refs/ hierarchy, while pseudo refs such as HEAD are exposed by iter_pseudo() instead. In any mixed transaction that also creates a packed_buffer, an existing loose HEAD is therefore missing from loose_names, so the new fast path asks packed.try_find("HEAD") and gets None; this makes PreviousValue::MustExist/MustExistAndMatch fail for an existing HEAD and lets MustNotExist incorrectly pass.
Useful? React with 👍 / 👎.
2c445bd to
ebd6541
Compare
|
Both review findings are addressed in the force-push (
I also applied the same trade-off gate raised on #2704: the loose-name snapshot is only built for transactions with at least 256 edits, so a small transaction against a store with many loose refs keeps the old O(edits) probing. The existing fast-path tests are padded past the threshold with filler creations so they still exercise it. All 181 gix-ref tests pass with sha1 and sha256 fixtures, clippy is clean. |
When verifying each edit's precondition, the transaction resolved the current value loose-first: `ref_contents()` (an `open()` syscall) then a packed fallback. On a freshly-packed store that probe is a futile ENOENT per ref, and a fetch on a ~155k-ref mirror does one per mapping — it dominated the wall time of applying the ref transaction. Acquire all per-ref locks first, then snapshot the packed buffer and the set of loose ref names once; names under `refs/` with no loose file resolve straight from packed-refs, skipping the `open()`. Taking the snapshot after every lock is held is what makes it sound: a cooperative writer must hold the same lock to change a ref, so the snapshot cannot go stale for any edited ref, and each CAS check still compares against a value that cannot change concurrently. Loose-over-packed precedence is preserved: any loose ref is in the snapshot and takes the original full lookup. Pseudo refs like `HEAD` live in the `.git` root rather than the `refs/` hierarchy the snapshot enumerates, so they always take the full lookup. Falls back to the full path when the snapshot is unavailable. This mirrors the existing negotiate / update_refs fast paths. The snapshot is only built for transactions with at least 256 edits: enumerating loose names walks the entire `refs/` tree, so a small transaction against a store with many loose refs would otherwise trade O(edits) probes for an O(all loose refs) scan. A `ref tx resolve` detail span records the resolution time (`find_ms`), matching the split-phase spans on the other two ref passes; it compiles out unless the `tracing-detail` feature is enabled. On the AUR mirror this roughly halved the post-fetch ref-update phase (~770ms -> ~400ms, resolution ~640ms -> ~240ms). Adds tests asserting the packed-only fast path's resolved value and loose-over-packed precedence in the pass and fail directions (padded past the snapshot threshold with filler creations), plus one asserting an existing `HEAD` is still seen by large transactions. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ebd6541 to
456e4f0
Compare
The two-pass rework that locks all edits before resolving their current values (the CAS-race fix) kept each update's `gix_lock::File` open until the second pass wrote its content, so a preparing transaction held one open file descriptor per edit. A fetch that updates thousands of refs — a large mirror after hours of churn — exceeded the default `ulimit -n 1024` and failed with EMFILE around the ~1000th lock. The lock file's content is the edit's *new* target, which is known before any resolution. Write it when the lock is acquired and close the handle immediately, keeping only a `gix_lock::Marker`: the lock file stays on disk for the whole preparation, so the lock-all-before-snapshot ordering (and with it the race protection) is unchanged, while the number of open file descriptors drops to O(1) instead of O(edits). The regression test lowers `RLIMIT_NOFILE` and prepares a 300-edit transaction; it fails with EMFILE before this change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed 66fdea3, fixing an fd-exhaustion regression the two-pass prepare introduced: pass 1 held every update's lock Since the lock file's content is the edit's new target — known before any resolution — it is now written at acquisition time and the handle closed immediately, keeping only a Regression test: Codex (@codex) review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66fdea31e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| let (_dir, store) = empty_store()?; | ||
| let edits: Vec<RefEdit> = (0..300).map(|i| create_at(&format!("refs/heads/fd-{i:03}"))).collect(); | ||
|
|
||
| let _guard = SoftLimitGuard::lower_to(96)?; |
There was a problem hiding this comment.
When this test is run under the default cargo test harness, tests in the same integration-test binary execute concurrently in threads, but setrlimit(RLIMIT_NOFILE) changes the limit for the entire process. During the prepare/commit window for these 300 refs, any sibling test that opens files can unexpectedly fail with EMFILE, making plain cargo test runs flaky; run this check in a subprocess or otherwise serialize it against the rest of the binary before lowering the limit.
Useful? React with 👍 / 👎.
| let loose_names: Option<std::collections::HashSet<FullName>> = packed_buffer | ||
| .filter(|_| updates.len() >= MIN_EDITS_FOR_LOOSE_NAME_SNAPSHOT) | ||
| .and(store.loose_iter().ok()) | ||
| .map(|iter| iter.filter_map(Result::ok).map(|r| r.name).collect()); |
There was a problem hiding this comment.
For large transactions that use the packed fast path, this silently omits any loose ref whose iterator item is an I/O/traversal error. If the edited ref is present as a loose file but cannot be read, the subsequent read_existing_ref() sees it as absent from loose_names and resolves from packed-refs instead of surfacing the read error as the old per-ref lookup did, so a CAS check can be made against a stale packed value and overwrite an unreadable loose ref.
Useful? React with 👍 / 👎.
The loose-name snapshot that large transactions use to fast-path packed-only resolution dropped error items from the loose-ref iterator (`filter_map(Result::ok)`). A loose ref that exists but cannot be enumerated — an unreadable file, or a directory-walk failure hiding a whole subtree — was thereby missing from the snapshot and treated as packed-only: its CAS check compared against the stale packed value and could pass, silently overwriting the loose ref that the old per-edit lookup would have refused to touch by surfacing the I/O error. Any enumeration error now disables the snapshot for the whole transaction, so every edit takes the plain per-edit lookup and I/O errors surface for exactly the refs an edit touches, as before this PR. Only stores with unreadable loose refs lose the fast path. The regression test makes a loose ref unreadable (mode 000) and asserts a 257-edit transaction fails with the permission error instead of matching the ref's stale packed value. Also isolate the fd-limit regression test's `RLIMIT_NOFILE` change: lowering the limit affects the whole process, and under plain `cargo test` sibling tests run as threads of that process and could spuriously hit EMFILE. The test now re-executes itself as a child process (marked by an environment variable) and only the disposable child lowers the limit; the parent asserts the child ran exactly this test and passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both round-2 findings are addressed in 6968545:
All 183 gix-ref tests pass with sha1 and sha256 fixtures under nextest, both changed tests also pass under plain Codex (@codex) review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69685456c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| // so pseudo refs like `HEAD` (which live in the `.git` root and are | ||
| // never packed) must always take the full lookup. | ||
| if let (Some(loose), Some(packed)) = (loose_names, packed) { | ||
| if name.as_bstr().starts_with(b"refs/") && !loose.contains(name) { |
There was a problem hiding this comment.
When a large transaction edits an existing loose worktree-private ref under refs/ (for example refs/worktree/foo, refs/bisect/foo, or refs/rewritten/foo), this condition can treat it as packed-only because store.loose_iter() is backed by LooseThenPacked::next, which skips names whose category is_worktree_private() in gix-ref/src/store/file/overlay_iter.rs. These categories are also not written to packed-refs by possibly_adjust_name_for_prefixes, so PreviousValue::MustExist* sees None and MustNotExist can incorrectly pass and overwrite the existing loose ref; route worktree-private categories through the full ref_contents() lookup like pseudo refs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don't think this one holds up — the is_worktree_private() skip in LooseThenPacked::next (advance_to_non_private) is applied only to the common-dir iterator; the git-dir iterator is peeked unfiltered on every path through peek_loose. And worktree-private categories are exactly the ones that resolve to git_dir: base_dir_and_rela_path maps Bisect | Rewritten | WorktreePrivate to (self.git_dir, name). So an existing loose refs/bisect/foo lives in the directory the unfiltered walk enumerates, lands in loose_names, and takes the full ref_contents() lookup — I verified with a probe test that a plain store with a loose refs/bisect/good yields it from store.loose_iter().
The linked-worktree case works out the same way: there git_dir is the worktree-private directory, so its refs/bisect/*/refs/worktree/*/refs/rewritten/* are yielded by the unfiltered walk. What advance_to_non_private skips are the main worktree's private refs in the common dir — which aren't addressable as refs/bisect/* from that store to begin with. And when a worktree-private ref doesn't exist loose, both paths converge: the fast path asks packed.try_find(name), and the slow path falls through to the same packed.try_find(name) after the loose miss — identical None, since these categories are never written to packed-refs (possibly_adjust_name_for_prefixes). Prefixed spellings like worktrees/X/refs/… or main-worktree/refs/… don't start with refs/, so the existing guard already routes them to the full lookup.
|
Codex (@codex) review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
When verifying each edit's precondition,
lock_ref_and_apply_changeresolves the current value loose-first:ref_contents()(anopen()syscall) with a packed fallback. On a freshly-packed store that probe is a futile ENOENT per ref, and a fetch on a ~155k-ref mirror does one per mapping — it dominated the wall time of applying the ref transaction.prepare_innernow snapshots the packed buffer and the set of loose ref names once; names with no loose file resolve straight from packed-refs insideread_existing_ref, skipping theopen(). Loose-over-packed precedence is preserved — any loose ref is in the snapshot and takes the original full lookup — and everything falls back to the full path when the snapshot is unavailable. This mirrors the fast paths already proposed for negotiate/update_refs (#2704).A
ref tx resolvedetail span records the resolution time (find_ms); it compiles out unless thetracing-detailfeature is enabled.On the AUR mirror this roughly halves the post-fetch ref-update phase (~770ms → ~400ms, resolution ~640ms → ~240ms).
Tests:
resolving_existing_values_reads_packed_only_refs_and_honors_loose_precedenceasserts both the packed-only fast path's resolved value and loose-over-packed precedence, in the pass and fail directions.Independent of #2704 and #2608 — applies directly to
main.🤖 Generated with Claude Code