Honor .ignore files, process watcher events off-thread, and parallelize the matcher walk - #95
Conversation
gitignore::build_matcher enumerated every .gitignore in the tree with a single-threaded WalkBuilder, while everything in walker.rs has long used threads(walker_thread_count()).build_parallel(). The walk is almost pure I/O wait, so one thread serializes every directory read. That made warm-start serve pathological on large network-backed trees. On a 289k-file repo on a network drive the matcher build took 205s, against 1.6s for the parallel stale-check walk over the same tree moments later -- a ~128x gap for the same traversal. The bootstrap path had already been worked around (it reuses outcome.gitignore_files from the indexing walk); only warm start still paid for a fresh serial walk. Switch the enumeration to build_parallel with a Mutex collector, keeping the existing filter_entry semantics byte for byte. Measured on the linux kernel tree (94,634 files, 403 .gitignore files), warm-start serve: before 1942.8ms after 363.9ms (5.3x) which brings it to parity with the parallel stale-check walk over the same tree (330ms), confirming the walk itself was the entire cost. Verified the parallel walk discovers exactly the same 403 .gitignore paths as the serial one by dumping and diffing both sets. Results are sorted before use. Not needed for correctness -- with_nested re-sorts deepest-first, which is what establishes git's between-level precedence -- but that re-sort is stable, so sorting keeps the built matcher reproducible run to run instead of varying with thread completion order. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9162aa2a-9639-412f-9c72-29d8418bb961
There was a problem hiding this comment.
Pull request overview
This PR parallelizes the .gitignore discovery walk performed by tgrep_core::gitignore::build_matcher, bringing it in line with the rest of the codebase’s parallel filesystem walking approach and significantly reducing warm-start latency on large or network-backed repositories.
Changes:
- Switch
.gitignoreenumeration from a single-threadedWalkBuilder::build()iterator tobuild_parallel()with a sharedMutex<Vec<PathBuf>>collector. - Add a dedicated thread-count helper (
matcher_walk_thread_count) and sort discovered paths to keep matcher construction reproducible across runs.
Show a summary per file
| File | Description |
|---|---|
| tgrep-core/src/gitignore.rs | Parallelize .gitignore enumeration during matcher construction; add thread-count helper and deterministic sorting. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
Incorporates #88 and #89, adapted to the current architecture. .ignore support (#88): the walker now collects `.ignore` alongside `.gitignore` and `IgnoreMatcher` applies both, with `.ignore` winning where the two collide. `.gitignore` is git-gated to match the indexing walk, so the watcher can no longer filter on rules the indexer never applied; `.ignore` applies with or without a repository. Off-thread watcher (#89): filesystem events are handed to a worker thread over a bounded queue instead of being indexed inside the notify callback. That callback runs on the platform's notification thread -- on Windows it owns a fixed-size ReadDirectoryChangesW buffer -- so doing file I/O and trigram extraction there stalls it and the OS silently drops whatever arrives meanwhile. On overflow the events are dropped deliberately and reconciled with a stale check once the burst drains. The bound is configurable with --watcher-queue-cap. Neither PR could be replayed as written. Both predate the nested IgnoreMatcher, and #88 deletes build_gitignore_matcher_after_ready, which is now also called from the ignore-rules-dirty loop -- applying it unchanged would have silently broken runtime .gitignore reloads. Both paths now route through background_refresh_stale. That also removes a redundant traversal: warm start built the matcher with its own full-tree walk, then walked again for the stale check. The matcher is now built from the stale walk's results. On a 94k-file tree that is a 616ms walk plus a 329ms walk down to a single 342ms walk with an 80ms matcher build. Fixes a related bug found while testing: ignore files matched by their own rules -- the common build/.gitignore holding `*` -- are filtered out of the walk's own output and so were never collected. The indexer still skipped the subtree, but the matcher did not know about it, so the watcher would index a subtree the indexer deliberately excluded. Walks now probe each directory they descend into rather than collecting the ignore files the walk yields. Co-authored-by: dpelksnitis <117392555+dpelksnitis@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9162aa2a-9639-412f-9c72-29d8418bb961
Flushing a changed file on a large index was dominated by `full_snapshot`,
not by disk I/O. On a 94k-file tree (170M posting entries, 0.97 GB index) a
single-file change spent 5.6s rebuilding the in-memory index before writing
a byte. At the scale of a 2.7 GB index that grows to ~38s.
Three causes, all in `full_snapshot`:
- The old -> new file id table was a `HashMap<u32, u32>`, consulted once per
posting entry. 170M random-u32 hashes cost 3.4s on their own. Reader ids
are the contiguous range `0..num_files`, so a dense `Vec` indexes directly
and the lookups run in file-id order, which is sequential rather than
scattered. 3425ms -> 76ms.
- The whole index was decoded into a `Vec<(u32, Vec<PostingEntry>)>` and only
then remapped, so every posting entry existed twice at peak. Decoding and
remapping now happen in a single fused pass, which removes the intermediate
copy entirely.
- That pass, and the final sort, are per-trigram independent, so both fan out
across cores.
Measured on the linux kernel tree, one changed file:
phase before after
decode postings 1353ms ┐
remap reader postings 3425ms ┘ 480ms (fused)
overlay merge 345ms 8ms
sort postings 376ms 27ms
full_snapshot 5581ms 565ms (9.9x)
total flush 9.5s 2.5s
Verified by rebuilding the same tree from scratch and diffing search results
against the incrementally-flushed index: identical output across six queries
covering 64k+ hits.
Note this only lowers the constant. A one-file change still costs
O(entire index) because `append_overlay_to_index`'s bounded-heap stream merge
requires append-only input, and a changed file supersedes a reader entry.
Teaching that merge to skip superseded file ids would remove the whole-index
work, and is the real fix; left as follow-up.
Also fold watcher-recorded stamps into the stale-check baseline
-------------------------------------------------------------
`filestamps.json` only advances when the index is flushed, but the watcher
updates `state.file_stamps` on every change, so mid-session the on-disk copy
lags. An overflow-triggered stale check reading only the on-disk stamps
re-indexes everything handled since the last flush, and a file created *and*
deleted inside that window appears in neither the stamps nor the filesystem,
so it is never classified as deleted and lingers in the index. The in-memory
stamps are the fresher record and now take precedence.
Co-authored-by: Dan Elksnitis <dpelksnitis@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9162aa2a-9639-412f-9c72-29d8418bb961
- Share the walk thread count. `matcher_walk_thread_count` was a byte-for-byte copy of `walker_thread_count`, so tuning one would silently desync the two full-tree walks. `walker_thread_count` is now `pub(crate)` and both use it. - Fix a stale doc comment on `walk_file_metadata`. It still described the `hidden(false)` + manual-dot-filtering approach that directory probing replaced; the walk uses `hidden(true)` and finds ignore files by probing each directory it descends into. - Stop `--watcher-queue-cap` truncating. The value was parsed as `u64` and cast with `as usize`, which on a 32-bit target silently wraps — and wrapping to 0 turns the watcher's `sync_channel` into a rendezvous channel where every `try_send` fails and no event is ever delivered. clap has no ranged `usize` parser, so the range is bounded by `usize::MAX` instead (lossless on both 32- and 64-bit) and the conversion saturates rather than wraps. Verified: `--watcher-queue-cap 0` and values past the bound are now rejected by clap. The rationale for the bound is a plain `//` comment so it stays out of `--help`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9162aa2a-9639-412f-9c72-29d8418bb961
There was a problem hiding this comment.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
tgrep-cli/src/main.rs:428
watcher_queue_cap.map(|n| n as usize)can silently truncate on 32-bit targets (and for very large values), potentially creating an unexpectedly small queue cap. Prefer parsingwatcher_queue_capasusizein clap (or useusize::try_from(n)and surface an error) so invalid values are rejected instead of truncated.
serve::run(
tgrep-core/src/walker.rs:218
- The doc comment says hidden entries are visited so
.gitignore/.ignoreare seen, but this walk usesWalkBuilder::hidden(true)(hidden entries skipped). Ignore files are still discovered via the per-directoryignore_files_inprobe, so this comment is misleading. Update it to describe the probing behavior (and that hidden dirs/files are still skipped for the metadata set).
/// Walk a directory tree collecting filesystem metadata (mtime, size) plus the
/// `.gitignore` / `.ignore` files encountered. No file content is read — this
/// is used for stale file detection on startup.
tgrep-cli/src/serve.rs:435
- This comment claims “Every exit path clears the gate”, but
publish_watcher_matcherreturns early when!state.watch_enabledwithout clearinggitignore_pending. Either adjust the wording (e.g., only when watching is enabled) or explicitly clear the gate on that early-return path to keep the comment and behavior aligned.
/// Every exit path clears the gate. A repo with no ignore rules at all yields
/// `None`, which is a legitimate final answer rather than a missing matcher, so
/// it clears the gate too — otherwise the watcher would stay muted forever.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
The v1.0.1 release is being re-cut to include the changes on this branch, so the workspace version goes back to 1.0.1. Cargo.lock is regenerated to match, since CI builds with --locked. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9162aa2a-9639-412f-9c72-29d8418bb961
There was a problem hiding this comment.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
tgrep-core/src/walker.rs:281
walk_file_metadataprobes every descended directory withgitignore::ignore_files_in(...)unconditionally. Whenno_ignoreis true, the caller will never build/use an ignore matcher, so these extra filesystem stats are pure overhead. Consider guarding the probe withif !no_ignore { ... }(or otherwise making ignore-file collection conditional) so--no-ignoremetadata walks stay as cheap as possible.
if should_skip_dir(&entry, &exclude) {
return ignore::WalkState::Skip;
}
// Probing each descended directory finds ignore files the walk
// itself filters out; see `gitignore::ignore_files_in`. It also
// keeps `hidden(true)` intact, so the metadata set is unchanged.
let (gitignore, dot_ignore) = crate::gitignore::ignore_files_in(entry.path());
if let Some(path) = gitignore {
gitignore_files.lock().unwrap().push(path);
}
if let Some(path) = dot_ignore {
ignore_files.lock().unwrap().push(path);
}
tgrep-cli/src/serve.rs:427
- The docstring for
publish_watcher_matchersays “Every exit path clears the gate”, but the early return when!state.watch_enableddoes not cleargitignore_pending. Either update the comment to scope that guarantee to the watch-enabled case, or clear the flag on that early return for consistency.
/// Publish the watcher's ignore matcher from ignore files discovered by a walk
/// that already happened, and release the `gitignore_pending` gate.
///
/// Reusing the caller's walk is the whole point: `gitignore::build_matcher`
/// would traverse the entire tree a second time purely to find the same ignore
/// files. On a 289k-file repo on a network drive that second walk cost 205s,
/// against 1.6s for the stale-check walk over the same tree.
///
/// Every exit path clears the gate. A repo with no ignore rules at all yields
/// `None`, which is a legitimate final answer rather than a missing matcher, so
/// it clears the gate too — otherwise the watcher would stay muted forever.
fn publish_watcher_matcher(
state: &ServerState,
root: &Path,
walk: &tgrep_core::walker::MetaWalkResult,
) {
if !state.watch_enabled {
return;
}
tgrep-core/src/gitignore.rs:420
build_matcher’sfilter_entrycurrently runs p4ignore matching on every file entry to decide whether to yield it, but the parallel walk no longer consumes file entries at all (it only probes directories viaignore_files_ininsidewalker.run). Consider short-circuiting file entries infilter_entry(e.g.,if !is_dir { return false; }) so the enumeration walk yields only directories and avoids per-file p4ignore work that can dominate on large trees.
if entry.file_type().is_some_and(|ft| ft.is_dir())
&& entry
.file_name()
.to_str()
.is_some_and(|n| n.starts_with('.'))
{
return false;
}
if entry.file_name() == ".gitignore" || entry.file_name() == ".ignore" {
return true;
}
- Files reviewed: 9/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
tgrep-cli/src/serve.rs:427
publish_watcher_matcher’s doc comment says “Every exit path clears the gate”, but theif !state.watch_enabled { return; }early return leavesgitignore_pendingunchanged. Either cleargitignore_pendingin that branch too, or adjust the comment to reflect that the gate is only relevant when watching is enabled.
/// Every exit path clears the gate. A repo with no ignore rules at all yields
/// `None`, which is a legitimate final answer rather than a missing matcher, so
/// it clears the gate too — otherwise the watcher would stay muted forever.
fn publish_watcher_matcher(
state: &ServerState,
root: &Path,
walk: &tgrep_core::walker::MetaWalkResult,
) {
if !state.watch_enabled {
return;
}
tgrep-core/src/gitignore.rs:380
build_matcher’s rustdoc still describes this as enumerating “every.gitignorefile inside the tree” viaWalkBuilder. The implementation now (a) collects.ignoreas well, and (b) probes each descended directory viaignore_files_inrather than relying on yielded ignore-file entries. Updating the docs to match the probing approach would prevent future refactors from reintroducing the “hidden(true) would hide .gitignore” assumption.
/// Build a `Gitignore` matcher rooted at `root`, mirroring the same
/// ignore semantics that `walker::walk_dir` / `walker::walk_file_metadata`
/// apply during iteration. Loads:
/// * `.git/info/exclude` (if present)
/// * every `.gitignore` file inside the tree
/// * the user's global gitignore (via `GitignoreBuilder`'s defaults)
/// * root-level `p4ignore.ini`
///
/// Uses `WalkBuilder` to enumerate `.gitignore` files so we automatically
/// skip the `.git` dir and gitignored subtrees while collecting rules.
/// Returns `None` when no rules could be loaded.
- Files reviewed: 9/10 changed files
- Comments generated: 1
- Review effort level: Lite
A search that finds no index degrades to brute force without saying anything. On a large tree that is the difference between a sub-second query and a multi-minute one, so it reads as a hang rather than a misconfiguration. The common trigger is starting `serve --index-path <dir>` and then omitting the flag on the search. `--index-path` is a global clap arg, which means it is accepted on every subcommand, not remembered between invocations. The client looks for serve.json in the default location, finds nothing, and falls through to brute force without ever contacting the running server. Measured on a 94k-file tree: 45.9s versus 0.14s for the identical query and result set. The existing "Server unreachable" notice does not cover this, since it only fires when serve.json loads but the connection fails. Warn on stderr instead, with a hint tailored to whether --index-path was given. Stay silent for --no-index (brute force was requested) and for -q (documented as suppressing all output). Paths are printed without Windows' \\?\ extended-length prefix, which canonicalize adds.
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
tgrep-cli/src/serve.rs:425
- The doc comment says "Every exit path clears the gate", but when
!state.watch_enabledthe function returns without clearinggitignore_pending. Even if it’s currently initialized to false in that mode, the comment is inaccurate/misleading; update the wording or clear the gate consistently.
/// Every exit path clears the gate. A repo with no ignore rules at all yields
/// `None`, which is a legitimate final answer rather than a missing matcher, so
/// it clears the gate too — otherwise the watcher would stay muted forever.
fn publish_watcher_matcher(
state: &ServerState,
root: &Path,
walk: &tgrep_core::walker::MetaWalkResult,
) {
if !state.watch_enabled {
return;
}
Cargo.toml:7
- The workspace package version is being downgraded from 1.0.2 to 1.0.1. Unless this PR is intentionally reverting a release, this is likely a mistake and could break release/versioning expectations; consider keeping 1.0.2 or bumping forward instead of decreasing.
[workspace.package]
version = "1.0.1"
edition = "2024"
- Files reviewed: 10/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
Three related changes to how tgrep discovers and applies ignore rules, and how the file watcher consumes events. Supersedes #88 and #89 — both are incorporated here, adapted to the current architecture.
1. Parallelize the gitignore matcher walk
gitignore::build_matcherenumerated.gitignorefiles with a single-threadedWalkBuilder. The walk is almost pure I/O wait, so one thread serialized every directory read. Everything inwalker.rshad usedbuild_parallel()for a long time; this was the last serial full-tree walk.The cost was extreme on large or network-backed trees. From a user's trace on a 285k-file repo on a network drive:
Both traversed the same tree, seconds apart — a ~128× gap that is entirely thread count.
2. Honor
.ignorefiles (supersedes #88)The walker now collects
.ignorealongside.gitignore, andIgnoreMatcherapplies both. Where the two collide,.ignorewins — within a directory via the nested sort, and at the root by being added last..gitignoreis now git-gated, matching the indexing walk'srequire_gitdefault: it applies only inside a repository. Without this the watcher could filter on rules the indexer never applied..ignoreapplies either way.3. Process watcher events off-thread (supersedes #89)
Events are handed to a worker thread over a bounded queue rather than indexed inside the
notifycallback. That callback runs on the platform's notification thread — on Windows it owns a fixed-sizeReadDirectoryChangesWbuffer — so doing file I/O and trigram extraction there stalls it, and everything arriving meanwhile is dropped by the OS with no error we can observe.On overflow, events are dropped deliberately and reconciled with a stale check once the burst drains (a quiet interval, so one overflow doesn't trigger a stale check per queued event). The bound is configurable via
--watcher-queue-cap, default 16384.Why these couldn't be replayed as written
Both PRs predate the current nested
IgnoreMatcher, and #88's flat root-anchoredGitignorewould have regressed nested anchoring — its own description admits the mis-anchoring. Its.ignoreidea is grafted ontoIgnoreMatcherinstead.More seriously, #88 deletes
build_gitignore_matcher_after_ready, which is now also called from the ignore-rules-dirty loop added after that PR was opened. Applying it unchanged would have silently broken runtime.gitignorereloads. Both call sites now route throughbackground_refresh_stale.Bonus: one fewer full-tree walk on warm start
Warm start built the matcher with its own full-tree walk and then walked again for the stale check. The matcher is now built from the stale walk's results, so the second traversal is gone. This also required hoisting the walk above
background_refresh_stale's early returns, sogitignore_pendingis released even when the index turns out to be up to date.Bug found while testing: self-ignoring ignore files were dropped
An ignore file matched by its own rules — the common
build/.gitignorecontaining*— is filtered out of the walk's own output, so collecting the ignore files a walk yields silently drops it. The indexer still skipped the subtree (the walker reads the file even when it won't emit it), but the matcher built from those paths did not know about it, so the watcher would index a subtree the indexer deliberately excluded — exactly the divergenceshould_skip_watcher_pathexists to prevent.Walks now probe each directory they descend into instead. That is also complete (a
.gitignoreonly matters when the walk descends into its directory, and every such directory is yielded) and duplicate-free.This was masked in tests because the fixtures had no
.gitdirectory, which made the walker's own gitignore filtering inert.Measurements
Warm start against a 94,634-file tree (403
.gitignorefiles), pre-built index:Stale check reports the same 94,637 files before and after, confirming the metadata walk's result set is unchanged.
Verification
-D warnings.watcher_dot_ignore.rsasserts a file under an.ignored directory is never indexed and that an ordinary file created at the same moment is — the second assertion keeps a watcher that simply stopped working from passing the first. Confirmed non-vacuous by disabling.ignoresupport and watching it fail.build/.gitignorecase.Note on ordering
An earlier draft of this description claimed collection order affects ignore precedence. That is wrong:
IgnoreMatcher::with_nestedalready re-sorts deepest-first, which is what establishes git's between-level precedence. The explicit sort exists only because that re-sort is stable, so unsorted parallel-completion order would let the built matcher vary run to run.4. Make index flush ~10× faster and halve its peak memory
Diagnosed from the same user trace, where a single changed file cost 38s to flush on a 2.7 GB index:
None of that is disk I/O — it is all
HybridIndex::full_snapshotrebuilding the in-memory index before a byte is written. Three causes:HashMap<u32, u32>, consulted once per posting entry. 170M random-u32 hashes cost 3.4s on their own. Reader ids are the contiguous range0..num_files, so a denseVecindexes directly — and the lookups then run in file-id order, sequential rather than scattered.Vec<(u32, Vec<PostingEntry>)>and only then remapped, so every posting entry existed twice at peak (~1.4 GB × 2 on the tree below, ~7 GB at the user's scale). Decode and remap are now one fused pass, removing the intermediate copy.Measured on the linux kernel tree (94,634 files, 446,892 trigrams, 170,005,346 posting entries, 0.97 GB index), one changed file:
full_snapshotCorrectness check: rebuilt the same tree from scratch and diffed search results against the incrementally-flushed index — identical output across six queries covering 64k+ hits (
scheduler_tick,EXPORT_SYMBOL_GPL,struct task_struct,kmalloc,spin_lock_irqsave,netdev_priv).What this does not fix
It lowers the constant, not the complexity. A one-file change still costs O(entire index), because the bounded-heap stream merge in
append_overlay_to_indexrequires append-only input, and a changed file supersedes a reader entry. Teaching that merge to skip superseded file ids would remove the whole-index work entirely — that is the real fix, and it is left as follow-up.5. Fold watcher-recorded stamps into the stale-check baseline
Part of #89 that I initially and wrongly dismissed.
filestamps.jsononly advances on flush, but the watcher updatesstate.file_stampson every change, so mid-session the on-disk copy lags. An overflow-triggered stale check reading only the on-disk stamps re-indexes everything handled since the last flush — and a file created and deleted inside that window appears in neither the stamps nor the filesystem, so it is never classified as deleted and lingers in the index indefinitely. The in-memory stamps are the fresher record and now take precedence.