Tool performance improvements - #115
Conversation
🦋 Changeset detectedLatest commit: 736a903 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@agent-think can you fix the broken tests |
ReadableStream's default queue can pull a chunk as soon as the stream is created. Use a zero high water mark so blob reads begin only when a consumer requests data across both supported stream runtimes.
commit: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
🔍 Devin Review: 1 flag
Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
|
@agent-think can you fix the merge conflicts and push |
coalesceChanges resolved every touched inode with pathsOf, which walks vfs_dirents parent-by-parent issuing one statement per ancestor. That is O(N x depth) round-trips per push tick — ~74k for a 20k-node tree. Every one of those lookups already hit a covering index; the cost was the statement count, not the plan. Add pathsOfMany: one recursive CTE resolves an entire batch of inodes to all of their hardlink names, seeded from json_each over the inode list and walking child_inode -> parent_inode upward. Hardlinks that share a parent directory need the seed's (parent, name) pair as the grouping key, not the parent inode alone — keying on the parent collapsed /one.txt and /two.txt into /one.txt/two.txt. Covered by a regression test. Unreachable inodes produce no seed row and are absent from the result, matching pathsOf returning [] for them.
Recursive grep walked every directory even when callers only wanted source files. Add an `exclude` option and pass its relative globs to the find walker, which prunes matching directories before reading their files. Tests cover searches with and without exclusions, interaction with the inclusion glob, and confirm that pruned files are not read.
A recursive grep issued three statements per file: the recursive-CTE
path resolve inside readFile, the chunk range read, and the blob fetch.
The traversal had already read the inode and size from vfs_dirents, so
the resolve was pure duplicate work — and it was the most expensive of
the three.
Carry inode and size out of the find walk (internally only; find()'s
public {path, type} shape is unchanged) and add
readCommittedFileByInode, which pulls a committed file's bytes straight
from the chunk store. Open write buffers are still honoured so an
in-flight write is not missed; pending creates have no inode and keep
the resolve path, as does grepping a single file directly.
Line framing is shared with the streaming decoder's semantics: a
trailing fragment without a newline is still a line and an empty file
yields nothing. Verified byte-identical against the streaming path for
empty files, missing trailing newlines, CRLF, unicode, and matches
straddling a 512KiB chunk boundary, with and without context lines.
statements per file 3.0 -> 2.0 (the resolve CTE: 501 -> 1 for 500 files)
grep over 18,963 nodes 11412 ms -> 814 ms (14x)
The push tick reads tombstones with `WHERE rev > ? AND op = 'delete' GROUP BY path`. Neither existing index serves it: `vfs_changes_by_rev(rev)` can drive the range but leaves `GROUP BY path` to a sort, so the planner instead scans `vfs_changes_by_path` in path order and ignores the rev predicate. Add `vfs_changes_by_op_rev(op, rev)` with the equality column first and the range column second. Register it as the schema v7 to v8 migration because the sync cursor and operation tables already occupy versions 6 and 7. The migration only adds an index, so existing tombstones remain unchanged. Tests cover fresh and upgraded databases and confirm that SQLite uses the index for the tombstone query.
just-bash's find and grep are filesystem-agnostic: they walk through
IFileSystem, one readdirWithFileTypes per directory. Against the
workspace stub every one is an RPC to the DO, so a recursive command
over a tree containing node_modules costs hundreds to thousands of
round-trips before any matching work happens.
Give the adapter an opt-in prefetch scope. On the first listing inside
a scope it reads the whole subtree with one server-side find() and
answers later readdirWithFileTypes calls from that snapshot. The shell
entrypoint opens a scope only for commands that actually traverse
(find, grep -r), picked by prefetch-policy.
Correctness constraints, each covered by a test:
* the snapshot lives only for the command that opened it, so no
listing is ever reused across commands;
* every mutating adapter method drops it, so a walk that writes
re-reads instead of trusting the snapshot;
* paths outside the prefetched root fall through to a direct listing;
* a failed prefetch degrades to direct listings rather than failing
the command;
* dofs's find reports "symlink" at runtime even though the published
type is narrower, so links keep isSymbolicLink rather than being
mislabelled as files.
The policy declines mutating traversals (-delete, -exec) where a
snapshot taken before the walk could describe entries the command
removes, and declines non-recursive greps that would not benefit.
find / 412 -> 9 stub calls (45.8x)
grep -rl / 832 -> 429 (1.9x; the traversal is gone, per-file reads
remain)
Output is byte-identical with the scope on and off in every case.
Content prefetch was prototyped and rejected: file bodies can only be
reconstructed from grep matches lossily (a file with no trailing
newline comes back with one), and silently altering file content is
not worth the remaining round-trips.
diffWith computed the full worktree status matrix and then discarded
everything outside opts.paths with makePathFilter. isomorphic-git can
prune the walk itself, so the traversal was visiting — and stat'ing and
hashing — the entire tree to produce a result the caller had already
narrowed.
Pass the caller's paths through as filepaths. The library's matching
rule (exact path or directory prefix) is the same one makePathFilter
implements, so the filter stays the authority on what is emitted and
this is purely a traversal hint. Callers that name no paths keep
isomorphic-git's default of walking everything.
This only became expensive once node_modules was synced into the
worktree: a repo whose .gitignore does not cover it makes every scoped
diff pay for the vendor tree.
300-package vendor tree, :
unscoped 294 ms, 4,865 fs calls
scoped 14 ms, 64 fs calls (21x, 76x fewer calls)
Scoped rows verified identical to the full run's src subset.
Prevent an invalidated in-flight subtree load from publishing a stale directory snapshot after a concurrent mutation. A generation check now limits cache publication to the active prefetch scope.
Keep the path-resolution savings from inode-based grep reads without materializing whole files. Chunk bytes are loaded only as the line scanner pulls them, preserving bounded memory use for large files.
Document the shared relative-glob exclusion behavior for find and grep, and add release metadata for the large-tree performance changes.
ae9166f to
736a903
Compare
There was a problem hiding this comment.
Devin Review found 2 new potential issues.
6 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| if (this.#withinPrefetch(normalized)) { | ||
| const cache = await this.#ensurePrefetch(); | ||
| const hit = cache?.get(normalized); | ||
| if (hit !== undefined) return hit.map((entry) => ({ ...entry })); |
There was a problem hiding this comment.
🟡 Successful moves leave traversal stale
After mv succeeds during a prefetched command, readdirWithFileTypes returns the old tree. The successful mv rename bypasses #invalidatePrefetch. Later traversals report the source present and destination absent.
Learn more
A prefetch scope spans the complete shell command, including command lists and pipelines. The first recursive traversal loads one directory snapshot. A later mv normally succeeds through the direct rename path, which never calls #invalidatePrefetch. Any subsequent traversal in the same shell command therefore reads the original snapshot.
Example: find .; mv old.txt new.txt; find . loads the cache during the first find. The move succeeds, but the second find still lists old.txt and omits new.txt.
Recommended fix: Call #invalidatePrefetch() before the first rename attempt in mv, matching every other mutating adapter method. Add a test covering a successful move between two cached listings.
Was this helpful? React with 👍 or 👎 to provide feedback.
| } catch { | ||
| // A failed prefetch must not fail the command: fall back to | ||
| // direct listings for the rest of the scope. | ||
| if (this.#prefetchLoad === load) this.#prefetchLoad = undefined; | ||
| return undefined; |
There was a problem hiding this comment.
🟡 Failed prefetch retries every directory
When #fs.find fails but direct listings work, each readdirWithFileTypes retries the full subtree. Resetting #prefetchLoad forgets the failure. Recursive commands can issue one failed subtree RPC per directory.
Learn more
The fallback continues the walk through direct readdir calls. Each child directory re-enters #ensurePrefetch, sees no load or cache, and launches the same find(root) again. This is especially costly when the one-shot subtree response exceeds an RPC limit while smaller directory listings remain usable.
Example: A tree has 10,000 directories. If find("/") rejects while readdir succeeds, a recursive find can attempt that failing whole-tree request roughly 10,000 times instead of once.
Recommended fix: Record a failed-prefetch sentinel for the current generation. Return undefined without retrying until invalidation or endPrefetch resets the scope. Add a test where find always rejects and verify a multi-directory walk calls it once.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Since #113
node_moduleshas now become a default part of workspace sync. Performance of routine file operations, grep/find/git etc have degraded. A sync pass looked up paths one at a time, recursive searches visited directories the caller did not care about, the Worker shell asked the durable object for each directory separately, and a focused Git diff still inspected the whole project. These costs add up quickly after a package install.This change makes those operations focus on the relevant part of the workspace. Sync resolves paths in groups and can find recent deletions without searching the full history. Recursive shell commands reuse one fresh directory listing for the command, while writes clear that listing so later reads stay current. Grep reuses information collected during its directory walk and still reads large files a piece at a time. A Git diff limited to a path now avoids walking unrelated directories.
The user-facing addition is an
excludeoption forfindandgrep. It matches a complete directory or file name, sonode_modulesis skipped whilenode_modules_extrais not.On a test workspace with 18,963 entries, excluding the package directory reduced
findfrom about 143 ms to under 1 ms and reducedgrepfrom about 11 seconds to about 4 ms. A scoped Git diff over a project with 300 packages fell from 294 ms to 14 ms. These numbers are workload-specific, but they show that the commands now avoid the unrelated work rather than making the same walk faster by a small amount.The tests cover grouped path lookup, files with more than one name, upgrades from existing workspaces, excluded directories, matches that cross file chunks, large-file reading, fresh directory listings after writes, and focused Git diffs. The filesystem documentation now describes the new option and its exact-name behavior, and the changeset records it for the next release.