Conversation
Summary: `FallbackWatcher`'s `recReaddir` is the only consumer of `walker` (unmaintained since 2021, three deps) in Metro. `walker` schedules an `lstat` for every entry the moment the parent directory is read, so a crawl has as many in-flight `fs` requests as the tree has entries and never applies backpressure. Those requests queue against a libuv thread pool of 4, so the concurrency buys nothing and the queued request state is pure overhead. This diff replaces it with a plain `async` `recReaddir` - a depth-first stack drained by a pool of at most `CRAWL_CONCURRENCY` (32) concurrent visits. The syscalls are identical, one `lstat` per entry and one `readdir` per directory, and so are the filter semantics: `ignored` is still evaluated against a directory before it's descended into or reported. `endCallback` goes away in favour of awaiting, and `normalizeProxy` also goes because `path.join` already normalises. The win32 settling delay is preserved and moves up into `startWatching`, which is the only place it was used. Crawl of this repo's `node_modules`, 33,394 entries (3,160 directories, 30,054 files, 180 symlinks), median of 5 runs, macOS 25.6 on APFS, node 22.13.1: | variant | wall (ms) | RSS (MB) | peak heap (MB) | max in-flight `readdir` | |---|---|---|---|---| | `walker` | 107.2 | 102.8 | 21.7 | 818 | | bounded, 16 | 107.4 | 66.3 | 11.4 | 16 | | bounded, 32 | 103.9 | 67.5 | 12.8 | 32 | | bounded, 64 | 100.9 | 71.8 | 14.5 | 64 | | bounded, 256 | 97.2 | 85.4 | 20.2 | 239 | Both report the same 3,160/30,054/180 and issue the same number of syscalls. Wall time is constrained by the libuv pool concurrency, so 32 is chosen for the memory: **-34% RSS and -41% peak heap** against `walker`. Changelog: Internal Test Plan: ``` yarn jest packages/metro-file-map yarn jest packages/metro/src/DeltaBundler/__tests__/resolver-test.js yarn flow check yarn lint ``` `packages/metro-file-map` is 599 tests over 28 suites, including `watchers/__tests__/integration-test.js`, which drives `FallbackWatcher` against a real temporary tree - new/changed/deleted files, symlinks to a file, to a directory and to a non-existent target, pre-existing files, a directory moved in from outside the watch root, a directory moved out, and deletion of a directory with files under it. `resolver-test` is the suite that needed the `fs` alias for `walker`, and it passes with that mock removed. Not covered: win32. The settling delay is unchanged in duration but it now runs in a different place, and there's no coverage of the `FallbackWatcher` path on Windows. The benchmark harness isn't part of this diff.
meta-cla
Bot
added
the
CLA Signed
robhogan
changed the title
walker dependency from FallbackWatcher, reduce RSS+heap
facebook-github-tools
Bot
added
the
Shared with Meta
Contributor
|
@vzaidman has imported this pull request. If you are a Meta employee, you can view this in D119076649. |
robhogan
changed the title
walker dependency from FallbackWatcher, reduce RSS+heap
This was referenced Sep 7, 2026
Closed
Contributor
robhogan
added a commit
that referenced
this pull request
Sep 7, 2026
Summary: `FallbackWatcher` starts an `fs.watch` on each directory from `recReaddir`'s `dirCallback`, and the crawl calls that after `readdir` has returned. Anything written into a directory *between the two* appears in neither the listing nor the watch, so it stays invisible until the next full crawl. There's no recovery on Linux or Windows. This is expo/expo#48950 - `npx expo install` against a running dev server leaves the new modules unresolvable until restart, which is a lot more painful than it sounds now that agents routinely run Metro in a VM. @brentvatne diagnosed it in expo/expo#49363, with a fix in Expo's fork that we can hopefully replace with this one. Now that we own the crawl (#1906), the fix is very simply to call `dirCallback` before `readdir` instead of after, which also moves it inside `recReaddir`'s existing `try`. Separate issues not fixed here, both pre-existing and both covered by expo/expo#49363 : - An `fs.watch` that emits `error` is never removed from `#watched`. Node emits no `close` after `error`, so the path can never be re-watched, and `#stopWatching` waits on a `close` that will not arrive. - On win32, `fs.watch` can report an event with no filename. `#detectChangedFile` drops it when `#dirRegistry[dir]` is empty, which is exactly the state a newly watched directory is in. Changelog: ``` - **[Fix]**: `FallbackWatcher` no longer misses files written to a directory while it is being crawled ``` Test Plan: ``` yarn jest packages/metro-file-map yarn flow check yarn lint ``` New `watchers/__tests__/FallbackWatcher-test.js` asserts the ordering directly, that `fs.watch` precedes `readdir` for the same directory, on the initial crawl and on a directory created while watching, plus that a directory whose `fs.watch` throws is skipped without failing the crawl. All three fail on the parent commit and pass here. The race can't be asserted behaviourally on macOS, because FSEvents delivers with a latency window and a watch started immediately after a write still reports it: ``` $ node -e "fs.writeFileSync(d+'/raced.js', ''); fs.watch(d, (e, f) => console.log(e, f))" rename raced.js ``` That's why the issue is Linux/Windows only, and why the assertion is on the ordering rather than on a missed event. Metro has no Windows coverage for this backend, so the win32 path is unexercised either way.
meta-codesync Bot
pushed a commit
that referenced
this pull request
Sep 9, 2026
…nux + Windows) watcher (#1907) Summary: `FallbackWatcher` starts an `fs.watch` on each directory from `recReaddir`'s `dirCallback`, and the crawl calls that after `readdir` has returned. Anything written into a directory *between the two* appears in neither the listing nor the watch, so it stays invisible until the next full crawl. There's no recovery on Linux or Windows. This is expo/expo#48950 - `npx expo install` against a running dev server leaves the new modules unresolvable until restart, which is a lot more painful than it sounds now that agents routinely run Metro in a VM. brentvatne diagnosed it in expo/expo#49363, with a fix in Expo's fork that we can hopefully replace with this one. Now that we own the crawl (#1906), the fix is very simply to call `dirCallback` before `readdir` instead of after, which also moves it inside `recReaddir`'s existing `try`. Separate issues not fixed here, both pre-existing and both covered by expo/expo#49363 : - An `fs.watch` that emits `error` is never removed from `#watched`. Node emits no `close` after `error`, so the path can never be re-watched, and `#stopWatching` waits on a `close` that will not arrive. - On win32, `fs.watch` can report an event with no filename. `#detectChangedFile` drops it when `#dirRegistry[dir]` is empty, which is exactly the state a newly watched directory is in. Changelog: ``` - **[Fix]**: `FallbackWatcher` no longer misses files written to a directory while it is being crawled ``` Pull Request resolved: #1907 Test Plan: ``` yarn jest packages/metro-file-map yarn flow check yarn lint ``` New `watchers/__tests__/FallbackWatcher-test.js` asserts the ordering directly, that `fs.watch` precedes `readdir` for the same directory, on the initial crawl and on a directory created while watching, plus that a directory whose `fs.watch` throws is skipped without failing the crawl. All three fail on the parent commit and pass here. The race can't be asserted behaviourally on macOS, because FSEvents delivers with a latency window and a watch started immediately after a write still reports it: ``` $ node -e "fs.writeFileSync(d+'/raced.js', ''); fs.watch(d, (e, f) => console.log(e, f))" rename raced.js ``` That's why the issue is Linux/Windows only, and why the assertion is on the ordering rather than on a missed event. Metro has no Windows coverage for this backend, so the win32 path is unexercised either way. Reviewed By: motiz88, javache Differential Revision: D119187785 Pulled By: vzaidman fbshipit-source-id: 5b69c83db65fa546a7f4ead5e4fb75b112cad9b9
meta-codesync Bot
pushed a commit
that referenced
this pull request
Sep 11, 2026
Summary: Publish Metro 0.87.1 from `main`. Changelog: [Internal] Pull Request resolved: #1926 Test Plan: ``` node scripts/updateVersion.js 0.87.1 sl diff --stat ``` Only the 17 `packages/*/package.json` manifests change, and every `0.87.0` reference is replaced. Draft release notes (from the 91 commits in v0.87.0...main - reverted and internal-only changes omitted): ``` - **[Feature]**: Add `resolver.schemeResolvers` to resolve URI-scheme-prefixed specifiers (eg `foo:bar`) with custom resolvers (#1804 by robhogan) - **[Feature]**: Resolve `metro:babel-runtime/<path>` imports to Metro's own `babel/runtime` dependency (8388f71 by robhogan) - **[Feature]**: `metro-file-map`: Add `crawlerFactory` to optionally replace the built-in Watchman / Node crawlers (68022f0 by vzaidman) - **[Feature]**: `metro-babel-transformer`: Pass `inlinePlatform` to Babel presets via `caller` (6bbe095 by robhogan) - **[Fix]**: Replace `image-size` dependency with vendored parsers, fix CVE alerts (#1860 by robhogan) - **[Fix]**: Include `charset=utf-8` in the `Content-Type` of text assets and source files served by the dev server (#1888 by robhogan) - **[Fix]**: Preserve `Platform.OS` write targets during constant inlining (#1890 by OskarEichler) - **[Fix]**: Inline the last duplicate key from static `Platform.select` object literals (#1889 by OskarEichler) - **[Fix]**: `FallbackWatcher` no longer misses files written to a directory while it is being crawled (#1907 by robhogan) - **[Fix]**: `HttpStore`: Handle socket errors during writes, so they're retried rather than crashing the process (fad3b88) - **[Fix]**: Treat `CI=false` and `CI=0` as not-CI when defaulting `watch`. CI is now detected from `process.env.CI` only, dropping the `ci-info` dependency (61e4592 by robhogan) - **[Fix]**: Fix Fast Refresh hitting undefined modules when using lazy (segmented) bundles in dev (c70d0ae by robhogan) - **[Performance]**: `FallbackWatcher`: Drop `walker` dependency, reduce crawl RSS by ~34% and peak heap by ~41% (#1906 by robhogan) - **[Performance]**: Don't emit redundant empty dependency map arrays for modules with no dependencies (#1858 by robhogan) - **[Types]**: Restore publishing of private (underscore-prefixed) fields in TypeScript declarations (#1876 by robhogan) - **[Types]**: Fix async functions being inferred as returning `void` (not `Promise<void>`), eg `MetroServer#end` (#1909 by robhogan) - **[Types]**: Tidy up generated types, allow nullable props to be omitted in more places (#1885 by robhogan) > NOTE: Experimental features are not covered by semver and can change at any time. - **[Experimental]**: Add `serializer.unstable_inlineDependencyMap` to inline module IDs at serialisation time (#1786 by robhogan) - **[Experimental]**: Add `serializer.unstable_getAsyncDependencyPath` to supply custom `paths` to framework-defined `__loadBundleAsync` (#1855 by robhogan) - **[Experimental]**: Remove `transformer.unstable_renameRequire` - `require` is never renamed (42577f7 by robhogan) - **[Experimental]**: `experimentalImportSupport`: Fix `export * from` re-exporting the source module's default export (#1777 by robhogan) **Full Changelog**: v0.87.0...v0.87.1 ``` Reviewed By: zeyap Differential Revision: D119675927 Pulled By: GijsWeterings fbshipit-source-id: daf17f3501b4725c21c39b4dfd5c82b95e01dca5
3 tasks
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
1 participant
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.
Summary:
FallbackWatcher'srecReaddiris the only consumer ofwalker(unmaintained since 2021, three deps) in Metro.walkerschedules anlstatfor every entry the moment the parent directory is read, so a crawl has as many in-flightfsrequests. Those requests queue against a libuv thread pool of 4, so the concurrency buys nothing and the queued request state is pure memory overhead. Note - I don't believe this is responsible forEMFILE: Too many open files, as libuv's limits should prevent that.This diff replaces it with a plain
asyncrecReaddir- a depth-first stack drained by a pool of at most 32 concurrent visits. The syscalls are identical, onelstatper entry and onereaddirper directory, and so are the filters:ignoredis still evaluated against a directory before it's descended into or reported.endCallbackgoes away in favour of awaiting, andnormalizeProxyalso goes becausepath.joinalready normalises.The win32 settling delay is preserved and moves up into
startWatching, which is the only place it was used.Crawl of this repo's
node_modules, 33,394 entries (3,160 directories, 30,054 files, 180 symlinks), median of 5 runs, macOS 25.6 on APFS, node 22.13.1:readdirwalkerBoth report the same 3,160/30,054/180 and issue the same number of syscalls. Wall time is constrained by the libuv pool concurrency, so 32 is chosen for the memory: -34% RSS and -41% peak heap against
walker.Changelog: Internal
Test Plan:
watchers/__tests__/integration-test.json all supported OSes, which drivesFallbackWatcheragainst a real temporary tree - new/changed/deleted files, symlinks to a file, to a directory and to a non-existent target, pre-existing files, a directory moved in from outside the watch root, a directory moved out, and deletion of a directory with files under it.