perf: parallelize file-change detection and speed up watcher startup#277
Open
Slicit wants to merge 4 commits into
Open
perf: parallelize file-change detection and speed up watcher startup#277Slicit wants to merge 4 commits into
Slicit wants to merge 4 commits into
Conversation
addRecursive walked the tree with filepath.Walk, which does an extra Lstat syscall per entry on top of the directory read. Switching to filepath.WalkDir avoids that, roughly halving the syscall count of the watch tree walk on large repos (100k+ files) -- this walk blocks watch startup/restart before any fsnotify events are served.
BenchmarkIndexAllWithProgress_BranchSwitchScenario only passed when b.N == 1: the mock store had no seeded documents, so the first iteration actually indexed all 800 files (populating it as a side effect), and only later iterations hit the mtime fast path. Any run with b.N > 1 (e.g. -benchtime=Nx) failed the benchmark's own assertion regardless of the indexer change in this PR. Pre-seeding documents fixes that. Also adds BenchmarkIndexAllWithProgress_FullHashRescan, which exercises the path the parallelization in indexer.go targets: every file's mtime looks new, so every file needs a real read+hash to discover it's actually unchanged.
IndexAllWithBatchProgress decided which files need (re)indexing one file at a time: a store lookup (GetDocument), an mtime fast-path gate, and -- for files that don't pass it -- a full read + SHA-256 hash. This is the exact phase that runs on every `grepai watch` restart, and after a git checkout/branch switch or CI clone where most files' mtimes look new, it becomes a full sequential hash of the repo before anything else can happen. decideFileScan now runs this per-file decision concurrently across a bounded worker pool (golang.org/x/sync/errgroup, matching the pattern already used in embedder/openai.go), sized via scanWorkerLimit (max(8, min(64, GOMAXPROCS*4))). Behavior, stats accounting, and the resulting file order are unchanged -- only the scheduling is concurrent. GetDocument is already safe for concurrent use on every backend: GOBStore guards its maps with an RWMutex, PostgresStore uses a pgxpool.Pool, and QdrantStore uses the qdrant client. prepareFileChunks (used by batch-capable embedders such as OpenAI/LM Studio) gets the same treatment: the per-file store delete + chunk generation loop is now concurrent, overlapping store round trips with chunking CPU work instead of paying both costs strictly one file at a time. Verified with go test -race ./indexer/... (no races) and the full test suite. Benchmarks in indexer_branchswitch_test.go: ~1.2x on a 2 vCPU sandbox for local hashing, ~7.9x for a simulated network-backed store (2000 files, 2ms/lookup) -- representative of Postgres/Qdrant, where the previous sequential loop paid N round trips back-to-back.
go test -race caught a real data race here: the indexer now calls GetDocument/DeleteByFile concurrently across files (see indexer.go), but mockStore mutated its maps with no locking at all. Real backends already handle this -- GOBStore has an RWMutex, Postgres uses a connection pool, Qdrant's client is safe for concurrent use -- so the test double needed to catch up to the same contract. Guarded every method with a mutex; no behavior change otherwise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Targets indexing/watch-restart speed on large repos (100k+ files, 5M+ LOC).
The problem: two phases here are fully sequential today.
Indexer.IndexAllWithBatchProgressdecides which files need (re)indexing before any embedding happens: for every scanned file it does a store lookup (GetDocument), an mtime fast-path check, and -- for files that don't pass it -- a full file read + SHA-256 hash. This runs one file at a time. It's exactly the phase that runs on everygrepai watchrestart (runInitialScan->IndexAllWithBatchProgress), and after a git checkout/branch switch or CI clone where most files' mtimes look "new," it becomes a full sequential hash of the entire repo before anything else can happen.watcher.addRecursiveusesfilepath.Walk, which does an extraLstatsyscall per file/directory on top of the directory read -- unnecessary work that roughly doubles the syscall count of the tree walkgrepai watchdoes at startup, before it can start serving file-change events.What changed:
indexer/indexer.go: the per-file "does this need (re)indexing" decision (decideFileScan) now runs concurrently across a bounded worker pool (golang.org/x/sync/errgroup, matching the pattern already used inembedder/openai.go), since each file's decision is independent of the others. Behavior, stats accounting, and the resulting file order are unchanged -- only the scheduling is concurrent. The same treatment is applied toprepareFileChunks(used by batch-capable embedders like OpenAI/LM Studio), which overlaps per-file store deletes with chunk generation instead of serializing them.watcher/watcher.go:addRecursiveswitched fromfilepath.Walktofilepath.WalkDir, removing a redundantLstatper entry during the watch tree walk.indexer/indexer_test.go:mockStoreis now mutex-guarded.go test -racecaught a real data race here once the indexer started calling store methods concurrently -- this brings the test double in line with the concurrency guarantees the real backends already provide (GOBStore'sRWMutex, Postgres's connection pool, Qdrant's client).indexer/indexer_branchswitch_test.go: fixedBenchmarkIndexAllWithProgress_BranchSwitchScenario, which only passed whenb.N == 1-- the mock store silently accumulated real documents on the benchmark's first iteration, so any run withb.N > 1(e.g.-benchtime=Nx) failed regardless of this PR. Also addedBenchmarkIndexAllWithProgress_FullHashRescan, which exercises the full-hash-comparison path this PR targets.Why this is safe:
GetDocument/DeleteByFileare already safe for concurrent use on every existing backend:GOBStoreguards its maps with anRWMutex,PostgresStoreuses apgxpool.Pool, andQdrantStoreuses the qdrant client -- all designed for concurrent access.Chunker.ChunkWithContextoperates on immutable config with no shared mutable state.framework.ProcessorRegistry.TransformForEmbeddingis read-only over its config/processor list;VueProcessoralready guards its own internal state with a mutex, indicating concurrent use was already anticipated.go test -race ./indexer/...-- no data races.go test ./...),go vet ./...is clean, and touched files aregofmt-clean.Benchmarks (2 vCPU sandbox -- gains scale with core count, and are largest for network-backed stores like Postgres/Qdrant where each
GetDocumentcall is a round trip rather than an in-memory map lookup):FullHashRescan-- 800 files, all need local read+hashNetworkLatencyRescan-- 2000 files, 2ms simulated store latency eachThe second scenario is the more representative stand-in for Postgres/Qdrant-backed projects, where the previous fully-sequential loop paid N round trips back-to-back; this PR overlaps them (bounded worker pool, sized
max(8, min(64, GOMAXPROCS*4))).Related Issue
Not tied to an existing issue -- opened directly after profiling indexing/watch-restart behavior on large repos.
Type of Change
How Has This Been Tested?
indexer/indexer_branchswitch_test.go, before/after comparison)All pass / clean.
Test Configuration: