Repository navigation
perf(gix): read directories ahead in status dirwalks - #3181
Andrea Cappuccio (hood) wants to merge 2 commits into
Conversation
Directory reads dominate walks, so threads read tracked directories ahead while the walk stays serial.
Use `thread_limit` for `prefetch_threads`: starship-like status is 38-56% faster.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 7df8113be9
ℹ️ 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".
| directories.sort_unstable(); | ||
| directories.dedup(); | ||
| Some(Prefetch { | ||
| listings: directories.iter().map(|_| OnceLock::new()).collect(), |
There was a problem hiding this comment.
With prefetching active (which Repository::index_worktree_status() now enables by default in parallel builds), each worker/get() stores a complete Vec in this OnceLock, and nothing removes a listing after readdir::recursive() consumes it. A full walk therefore ends with names and file types for essentially every entry in every tracked directory still resident, changing the walk from streaming memory to O(total worktree entries); large monorepos or tracked directories containing huge untracked sets can now spike or OOM. Use bounded lookahead and take/drop consumed listings.
Useful? React with 👍 / 👎.
| sync::{ | ||
| OnceLock, | ||
| atomic::{AtomicBool, AtomicUsize, Ordering}, | ||
| }, |
There was a problem hiding this comment.
The new worker-coordination state uses std::sync::OnceLock and atomics directly rather than the workspace's feature-aware interior-mutability abstraction, creating a separate synchronization implementation whose serial/parallel behavior can drift from the rest of the workspace. Wire the appropriate gix_parallel feature through gix-dir and use its shared-state primitives (or gate this module) instead.
AGENTS.md reference: AGENTS.md:L119-L119
Useful? React with 👍 / 👎.
| /// The traversal itself still happens on the calling thread, and its results are unaffected. | ||
| /// If `0`, the default, all directories are read by the calling thread. | ||
| /// It has no effect on walks [for deletion](Self::for_deletion). | ||
| pub prefetch_threads: usize, |
There was a problem hiding this comment.
Adding a field to this non-#[non_exhaustive] public options struct breaks every downstream gix_dir::walk::Options struct literal that does not supply it, but the reviewed commit is titled perf(gix): ..., so generated release notes assign it to the wrong crate and omit the required breaking marker. Split this API addition into a feat(gix-dir)! commit (with its adaptations) or otherwise give it the correct scope and breaking marker.
AGENTS.md reference: AGENTS.md:L98-L101
Useful? React with 👍 / 👎.
| pub(super) fn new(index: &'index gix_index::State, root: &BStr) -> Option<Self> { | ||
| let path_backing = index.path_backing(); | ||
| let mut directories = Vec::new(); | ||
| for entry in index.entries() { |
There was a problem hiding this comment.
For status requests with a narrow pathspec, gix-status/src/index_as_worktree_with_renames/mod.rs:131 still sets explicit_traversal_root to the whole worktree, so root here is empty and this loop materializes and sorts directories for every non-skip index entry. A one-path status in a million-entry repository therefore performs O(full index) setup and workers may issue unrelated read_dir calls even though the walker prunes those directories; pass the pathspec/common prefix into candidate selection so narrow status remains narrow.
Useful? React with 👍 / 👎.
| let path = worktree_root.join(rela_path); | ||
| listing.get_or_init(|| read(&path, precompose_unicode)); |
There was a problem hiding this comment.
If an indexed directory has been replaced by a symlink before status runs, this speculative read_dir follows the link before the serial walker classifies the entry and lets can_recurse reject it. The worker can consequently scan and cache an arbitrary external or slow/network directory that the normal walk would never enter; enqueue candidates only after traversal approves them, or verify every component without following symlinks before reading.
Useful? React with 👍 / 👎.
|
It's nice as a prototype, but would need much more work to get right. For one, the serial path can't be removed. |
Thanks for looking, and for fixing #3182 so quickly. Before anything gets reworked: which serial path do you mean?
Maybe the concern is something else instead:
Knowing which would tell whether a reworked version is worth proposing, or whether this is better left for you to do alongside #3153. |
Summary
statuswith untracked files now reads directories ahead on worker threads, while the walk itself stays serial and unchanged. On a starship-likestatus, this is 38-56% faster on everyday repositories, with byte-identical results.Motivation: shell prompts. starship's
git_statusmodule runsRepository::status()with untracked files enabled, once per prompt, and that time is dominated by the directory walk: on pnpm, ~250 of the 285 ms. The walk is syscall-bound (user 0.02 s, system 0.19 s for 3,562read_dircalls), so parallel reads pay off. Closes #3180.Changes
feat(gix-dir)!: newwalk::Options::prefetch_threads, defaulting to0(unchanged behavior).SKIP_WORKTREEentries) and keeps oneOnceLockper directory.get_or_init(): it uses a finished listing, waits for one in progress, or reads the directory itself. No directory is read twice.node_modulesare never read ahead.read_dir_errorsandread_dir_callsstay the same.walk(), which said traversal is rare in typical Git usage.perf(gix):Repository::index_worktree_status()setsprefetch_threadsfromthread_limit(gix_parallel::num_threads(), or0when that resolves to one thread).Repository::dirwalk()is unchanged.Results
Starship-like
status: untrackedMatching,TrackRenames::Disabled,thread_limit: Some(3)(starship's macOS default) orNone. Apple M1 Pro, macOS, hyperfine, 30 runs each, medians, local clones:thread_limitmainCPU time rises a little (pnpm with 3 threads: user 34 → 42 ms, system 229 → 351 ms), which is the cost of the extra reader threads.
This machine runs endpoint security software, which makes every syscall more expensive:
git statuson neovim takes ~40 ms here. Expect lower absolute times elsewhere. The relative gain depends on how expensiveread_diris.Correctness
walk::prefetching_directories_yields_the_same_outcome. It walks every repository of themany.shfixture withprefetch_threads0and3, across three emission configurations, with fresh and stale indices, and asserts equal entries andOutcome. When prefetched listings are deliberately truncated, the test fails.statusitems betweenmainand this branch on a dirty pnpm worktree:node_modulesat the root and nested, a symlink to a directory, and modified, deleted and type-changed tracked files.Matchingand collapsed emission were compared, with 3 threads and with all cores, 5 runs each.GIX_TEST_IGNORE_ARCHIVES=1, these pass on macOS:cargo test -p gix-dir --features sha1: 86 tests.cargo test -p gix-status --features sha1: 42 tests.cargo test -p gix: 682 tests.cargo clippyis clean for-p gix-dir -p gix-status -p gix --all-targets, and for-p gix --no-default-features --features status,sha1, wheregix-parallelis serial and prefetching is off.cargo +stable fmt --checkpasses.Reproduce
The harness is a ~60-line binary depending on
gixwithdefault-features = false, features = ["max-performance", "revision", "status", "sha1", "zlib-sirocco"](starship's set). It opens withThreadSafeRepository::discover_opts()and iteratesrepo.status(..)?.index_worktree_options_mut(|o| { o.thread_limit = Some(3); o.dirwalk_options.as_mut().map(|d| d.set_emit_untracked(Matching).set_emit_ignored(None).set_emit_pruned(false).set_emit_empty_directories(false)); }).tree_index_track_renames(Disabled).into_iter(None).