gix-protocol: skip the loose-ref probe when building the fetch have-set#2704
gix-protocol: skip the loose-ref probe when building the fetch have-set#2704Nik B (nikicat) wants to merge 3 commits into
Conversation
The per-mapping loop in mark_complete_and_common_ref is the dominant cost on repos with very many refs (the AUR mirror has ~155k mappings, ~5s here). It does one refs.find() plus one graph.get_or_insert_commit() per mapping, but nothing distinguished the two. Wrap the loop in a `mark mappings` detail span carrying `find_ms` / `commit_ms` totals so the packed-ref lookup cost and the ODB/commit-graph load cost can be told apart. The Instant reads are a few ns each, dwarfed by the work they bracket, so they stay unconditional; the recorded fields are no-ops when the gix-trace `tracing`/`tracing-detail` features are off. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> (cherry picked from commit cd6e5b9)
![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: e2c9dd4d33
ℹ️ 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".
| let loose_names: Option<std::collections::HashSet<bstr::BString>> = packed | ||
| .and(refs.loose_iter().ok()) | ||
| .map(|iter| iter.filter_map(Result::ok).map(|r| r.name.into()).collect()); |
There was a problem hiding this comment.
When a packed-refs file exists, this eagerly walks and parses every loose reference to build loose_names before resolving any mapping. Previously a fetch whose refspec mapped only a few refs only probed those local names via find, so repositories with many loose refs but small fetches regress from O(mapped refs) filesystem work to O(all loose refs) on every fetch. Consider keeping the old path for small mapping sets or using a lazy per-name loose check instead of collecting the whole loose namespace unconditionally.
Useful? React with 👍 / 👎.
e2c9dd4 to
2792b89
Compare
|
Addressed in the force-push ( One coverage note: with the gate, the small-fixture fetch tests no longer reach the snapshot path end-to-end — the packed-only lookup contract itself remains covered by the gix-ref |
`mark_complete_and_common_ref` resolves every mapping's local ref via
`Store::find`, which probes the loose ref file on disk *before* consulting
packed-refs. On a freshly-packed mirror (the AUR mirror has ~155k mappings)
that is one wasted `open()` syscall per ref: measured at ~2.1s of the
have-set build, which drops to ~0.6s once skipped.
Add `file::Store::try_find_packed_only`, which resolves a name from a
snapshotted packed buffer only, reusing `find_one_with_verified_input`'s
precompose/namespace/multi-directory logic but skipping the loose probe
(threaded through as a named `LooseRefs::{Consult,Skip}` rather than a bare
bool, so it can't be transposed with the adjacent `consider_pseudo_ref`).
In negotiate, snapshot packed-refs and the set of loose ref names once, then
take the packed-only path for names that aren't loose; names that *are* loose
still go through `find`, preserving loose-over-packed precedence. If either
snapshot is unavailable we fall back to `find` for everything. A missed loose
name would only cost negotiation efficiency, never correctness.
The snapshot is only built when the fetch maps at least 256 refs: enumerating
loose names walks the entire `refs/` tree, so a small fetch into a repository
with many loose refs would otherwise trade O(mapped refs) probes for an
O(all loose refs) scan. Below the threshold every name takes the plain `find`
path, exactly as before.
Tests:
- gix-ref: `try_find_packed_only::skips_loose_and_honors_packed` locks the
packed-only contract against a fixture with packed-only, loose-over-packed
(distinct OIDs), and loose-only refs.
- gix: `fetch_with_multi_round_negotiation` now runs each case with loose and
with packed refs, asserting identical rounds and pack object count.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`mark_complete_and_common_ref` only needs each local tracking ref's target oid to build the have-set, but resolved packed-only refs through `try_find_packed_only`, which allocates an owned `Reference` (name BString + target) per ref. On the AUR mirror (~155k mappings) that allocation dominated `find_ms`. Look the packed-only case up straight from the buffer with the borrowed `packed::Buffer::try_find` and read `target()` (packed refs are always direct). Loose refs still take the full loose-first lookup. Cuts mark mappings `find_ms` by ~120ms; behavior is unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> (cherry picked from commit ad63c23)
2792b89 to
01d2d3a
Compare
1 participant
mark_complete_and_common_refresolves every mapping's local ref viaStore::find, which probes the loose ref file on disk before consulting packed-refs. On a freshly-packed mirror that is one wastedopen()syscall per ref — on the AUR mirror (~155k tracking refs) the have-set build measures ~2.1s, dropping to ~0.6s with this change.Three commits:
mark_complete_and_common_ref's mapping loop cost — wraps the per-mapping loop in amark mappingsdetail span carryingfind_ms/commit_ms, so the packed-ref lookup cost and the ODB/commit-graph load cost can be told apart. Fields are no-ops unless the gix-tracetracing/tracing-detailfeatures are on.file::Store::try_find_packed_only— resolves a name from a snapshotted packed buffer only, reusingfind_one_with_verified_input's precompose/namespace/multi-directory logic but skipping the loose probe (threaded as a namedLooseRefs::{Consult,Skip}, not a bare bool). In negotiate, packed-refs and the loose-name set are snapshotted once; loose names keep the full loose-first lookup so precedence is preserved, and everything falls back tofindwhen either snapshot is unavailable — a missed loose name can only cost negotiation efficiency, never correctness.target()off the borrowedpacked::Buffer::try_findrecord instead of allocating an ownedReferenceper ref (~120ms offind_ms).Tests:
try_find_packed_only::skips_loose_and_honors_packedlocks the packed-only contract against a fixture with packed-only, loose-over-packed (distinct OIDs), and loose-only refs.fetch_with_multi_round_negotiationnow runs each case with loose and packed refs, asserting identical rounds and pack object counts.🤖 Generated with Claude Code