-
Notifications
You must be signed in to change notification settings - Fork 511
Tool performance improvements #115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
d3a044b
3d9ee98
56ab264
d041f09
898ff50
87fdf46
baaada6
5d42d5e
95028a0
736a903
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@cloudflare/computer": minor | ||
| --- | ||
|
|
||
| Add subtree exclusions to filesystem searches and reduce database and RPC work for sync, Worker shell tree walks, grep, and scoped Git diffs. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,10 +78,134 @@ type AdapterReadFileOptions = { encoding?: "utf8" | null } | "utf8" | null | und | |
| export class WorkspaceFsAdapter { | ||
| readonly #fs: WorkspaceFs; | ||
|
|
||
| // Directory-listing cache, populated by a single subtree read and | ||
| // consulted only while a prefetch scope is open. See beginPrefetch. | ||
| #prefetchDepth = 0; | ||
| #prefetchRoot: string | undefined; | ||
| #direntCache: Map<string, DirentEntry[]> | undefined; | ||
| #prefetchLoad: Promise<void> | undefined; | ||
| #prefetchGeneration = 0; | ||
|
|
||
| constructor(fs: WorkspaceFs) { | ||
| this.#fs = fs; | ||
| } | ||
|
|
||
| // --- Directory prefetch ------------------------------------------ | ||
| // | ||
| // just-bash's find and grep walk the tree with one | ||
| // readdirWithFileTypes per directory. Each is an RPC to the | ||
| // workspace, so a recursive command over a tree that includes | ||
| // node_modules costs thousands of round-trips before any matching | ||
| // work happens. | ||
| // | ||
| // A caller that is about to run such a walk can open a prefetch | ||
| // scope: the adapter reads the whole subtree once via the | ||
| // workspace's server-side find() and answers subsequent | ||
| // readdirWithFileTypes calls from that snapshot. | ||
| // | ||
| // Correctness rules: | ||
| // * The cache lives only for the duration of the scope. Nothing | ||
| // persists between commands, so a later command never sees a | ||
| // stale tree. | ||
| // * Any mutation through the adapter drops the cache immediately, | ||
| // so a walk that writes (find -delete, a pipeline that edits | ||
| // files) re-reads rather than trusting the snapshot. | ||
| // * Paths outside the prefetched root fall through to a direct | ||
| // listing. | ||
| // | ||
| // Scopes nest: only the outermost begin/end pair drives the load and | ||
| // the teardown, so a caller can open a scope without knowing whether | ||
| // one is already active. | ||
| beginPrefetch(root: string): void { | ||
| this.#prefetchDepth += 1; | ||
| if (this.#prefetchDepth > 1) return; | ||
| this.#prefetchGeneration += 1; | ||
| this.#prefetchRoot = normalizePath(root); | ||
| this.#direntCache = undefined; | ||
| this.#prefetchLoad = undefined; | ||
| } | ||
|
|
||
| endPrefetch(): void { | ||
| if (this.#prefetchDepth === 0) return; | ||
| this.#prefetchDepth -= 1; | ||
| if (this.#prefetchDepth > 0) return; | ||
| this.#prefetchGeneration += 1; | ||
| this.#prefetchRoot = undefined; | ||
| this.#direntCache = undefined; | ||
| this.#prefetchLoad = undefined; | ||
| } | ||
|
|
||
| // Drop the snapshot. Called from every mutating method so a write | ||
| // inside a prefetch scope is never masked by cached listings. | ||
| #invalidatePrefetch(): void { | ||
| this.#prefetchGeneration += 1; | ||
| this.#direntCache = undefined; | ||
| this.#prefetchLoad = undefined; | ||
| } | ||
|
|
||
| // True when `path` sits inside the active prefetch root. | ||
| #withinPrefetch(path: string): boolean { | ||
| if (this.#prefetchDepth === 0 || this.#prefetchRoot === undefined) return false; | ||
| if (this.#prefetchRoot === "/") return true; | ||
| return path === this.#prefetchRoot || path.startsWith(`${this.#prefetchRoot}/`); | ||
| } | ||
|
|
||
| // Build the directory -> entries map for the whole prefetched | ||
| // subtree from one find() call. Concurrent callers share the load. | ||
| async #ensurePrefetch(): Promise<Map<string, DirentEntry[]> | undefined> { | ||
| const root = this.#prefetchRoot; | ||
| if (root === undefined) return undefined; | ||
| if (this.#direntCache !== undefined) return this.#direntCache; | ||
| if (this.#prefetchLoad === undefined) { | ||
| const generation = this.#prefetchGeneration; | ||
| this.#prefetchLoad = (async () => { | ||
| const found = await this.#fs.find(root); | ||
| const cache = new Map<string, DirentEntry[]>(); | ||
| // The root itself always exists as a (possibly empty) bucket so | ||
| // an empty directory reads as empty rather than missing. | ||
| cache.set(root, []); | ||
| for (const entry of found) { | ||
| const slash = entry.path.lastIndexOf("/"); | ||
| const parent = slash <= 0 ? "/" : entry.path.slice(0, slash); | ||
| const name = entry.path.slice(slash + 1); | ||
| let bucket = cache.get(parent); | ||
| if (bucket === undefined) { | ||
| bucket = []; | ||
| cache.set(parent, bucket); | ||
| } | ||
| // dofs's find reports "symlink" at runtime even though the | ||
| // published WorkspaceFoundEntry union is narrower, so widen | ||
| // here rather than mislabelling links as regular files. | ||
| const type = entry.type as "file" | "dir" | "symlink"; | ||
| const isDirectory = type === "dir"; | ||
| const isSymbolicLink = type === "symlink"; | ||
| bucket.push({ | ||
| name, | ||
| isFile: !isDirectory && !isSymbolicLink, | ||
| isDirectory, | ||
| isSymbolicLink, | ||
| }); | ||
| if (isDirectory && !cache.has(entry.path)) cache.set(entry.path, []); | ||
| } | ||
| // A mutation may have invalidated this load while find() was | ||
| // in flight. Only the current generation may publish a cache. | ||
| if (this.#prefetchGeneration === generation && this.#prefetchRoot === root) { | ||
| this.#direntCache = cache; | ||
| } | ||
| })(); | ||
| } | ||
| const load = this.#prefetchLoad; | ||
| try { | ||
| await load; | ||
| } 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; | ||
| } | ||
| return this.#direntCache; | ||
| } | ||
|
|
||
| // --- Reads ------------------------------------------------------- | ||
|
|
||
| async readFile(path: string, _options?: AdapterReadFileOptions): Promise<string> { | ||
|
|
@@ -131,6 +255,18 @@ export class WorkspaceFsAdapter { | |
| if (isDevDir(path)) { | ||
| return [{ name: "null", isFile: true, isDirectory: false, isSymbolicLink: false }]; | ||
| } | ||
| const normalized = normalizePath(path); | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Successful moves leave traversal stale After Learn moreA prefetch scope spans the complete shell command, including command lists and pipelines. The first recursive traversal loads one directory snapshot. A later Example: Recommended fix: Call Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| if (cache !== undefined) { | ||
| // Inside the prefetched subtree but absent from the snapshot: | ||
| // the directory does not exist. Surface the same error a direct | ||
| // listing would. | ||
| throw createWorkspaceError("ENOENT", "no such file or directory", path); | ||
| } | ||
| } | ||
| const entries = await this.#fs.readdir(path); | ||
| return entries.map((e) => ({ | ||
| name: e.name, | ||
|
|
@@ -151,11 +287,13 @@ export class WorkspaceFsAdapter { | |
|
|
||
| async writeFile(path: string, content: string | Uint8Array, _options?: unknown): Promise<void> { | ||
| if (isDevNull(path)) return; | ||
| this.#invalidatePrefetch(); | ||
| await this.#fs.writeFile(path, content); | ||
| } | ||
|
|
||
| async appendFile(path: string, content: string | Uint8Array, _options?: unknown): Promise<void> { | ||
| if (isDevNull(path)) return; | ||
| this.#invalidatePrefetch(); | ||
| let existing: Uint8Array; | ||
| try { | ||
| const stream = await this.#fs.readFile(path); | ||
|
|
@@ -173,29 +311,34 @@ export class WorkspaceFsAdapter { | |
|
|
||
| async mkdir(path: string, options?: { recursive?: boolean }): Promise<void> { | ||
| if (isDevDir(path)) return; | ||
| this.#invalidatePrefetch(); | ||
| await this.#fs.mkdir(path, options); | ||
| } | ||
|
|
||
| async rm(path: string, options?: { recursive?: boolean; force?: boolean }): Promise<void> { | ||
| if (isVirtualDevPath(path)) return; | ||
| this.#invalidatePrefetch(); | ||
| await this.#fs.rm(path, options); | ||
| } | ||
|
|
||
| async chmod(path: string, mode: number): Promise<void> { | ||
| if (isVirtualDevPath(path)) return; | ||
| this.#invalidatePrefetch(); | ||
| await this.#fs.chmod(path, mode); | ||
| } | ||
|
|
||
| async symlink(target: string, linkPath: string): Promise<void> { | ||
| if (isVirtualDevPath(linkPath)) { | ||
| throw createWorkspaceError("EEXIST", "file exists", linkPath); | ||
| } | ||
| this.#invalidatePrefetch(); | ||
| await this.#fs.symlink(target, linkPath); | ||
| } | ||
|
|
||
| // --- Composites -------------------------------------------------- | ||
|
|
||
| async cp(src: string, dest: string, options?: { recursive?: boolean }): Promise<void> { | ||
| this.#invalidatePrefetch(); | ||
| const recursive = options?.recursive === true; | ||
| const s = await this.#fs.stat(src); | ||
| if (s.isDirectory) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Failed prefetch retries every directory
When
#fs.findfails but direct listings work, eachreaddirWithFileTypesretries the full subtree. Resetting#prefetchLoadforgets the failure. Recursive commands can issue one failed subtree RPC per directory.Learn more
The fallback continues the walk through direct
readdircalls. Each child directory re-enters#ensurePrefetch, sees no load or cache, and launches the samefind(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 whilereaddirsucceeds, a recursivefindcan 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
undefinedwithout retrying until invalidation orendPrefetchresets the scope. Add a test wherefindalways rejects and verify a multi-directory walk calls it once.Was this helpful? React with 👍 or 👎 to provide feedback.