Repository navigation
refactor(metro-file-map): Pre-resolve symlink targets and store normal posix paths - #1687
Conversation
meta-cla
Bot
added
the
CLA Signed
facebook-github-tools
Bot
added
the
Shared with Meta
| const cachedResult = this.#cachedNormalSymlinkTargets.get(symlinkNode); | ||
| if (cachedResult != null) { | ||
| return cachedResult; | ||
| } |
There was a problem hiding this comment.
On the face of it, this looks like it could be a perf regression, because we're introducing a new allocation and some string manipulation every time the same symlink is traversed, vs once per symlink.
I'd be interested to see a benchmark on a symlink-heavy project. Weakmap lookups should generally be very fast.
There was a problem hiding this comment.
this was admittedly tied to the on-demand file system change, which necessitated changes here anyway. It then made sense to pre-normalise values which ended up being a net neutral change (performance-wise) across all combined changes (!)
I can give benchmarking this in isolation a go next week 👍
Together with the lazy symlink resolution I would expect this to be mainly a neutral change since the old code paths assumed a low amount of symlinks. But I don't have an intuition of what this would look like in a project with many symlinks where the cost isn't negligible. My intuition overall with this change was that the cost of WeakMap lookups can add up faster than computing the path once, provided symlink resolution is lazier than it is now
There was a problem hiding this comment.
This seems to show that it's mostly neutral: kitten@eee7a82
Intuitively, the same amount of work happens (at least for macOS), since the path is prenormalised and stored in the file tree data structure, which means that the work (which was negligible to begin with) is now happening earlier and makes the WeakMap obsolete.
I didn't test this on Windows, where presumably there's a small additional cost due to the path normalisation (win -> posix -> win), which keeps the file map data structure portable. We could simply skip this and keep this denormalised, but the parity is nice imo
| ancestorOfRootIdx: this.#pathUtils.getAncestorOfRootIdx(symlinkTarget), | ||
| normalPath: symlinkTarget, | ||
| startOfBasenameIdx: symlinkTarget.lastIndexOf(path.sep) + 1, |
There was a problem hiding this comment.
If we're not caching, we may be better off computing some of this lazily (and eg, just return the normalPath here).
There was a problem hiding this comment.
Removed the intermediate data structure in 0a64351
The WeakMap is likely more expensive and this can leave unnormalized absolute paths in the file map cache. NOTE: This bumps the `CACHE_BREAKER` value!
This keeps the metro-file-map value system-stable. Before they could store absolute paths, and system paths, however, this is now normalized to posix normal paths.
32f7bd4 to
4bd59ba
Compare
`resolveSymlinkToNormal` trimmed a trailing separator from the readlink result before testing whether it was absolute. For a filesystem root that separator is the only thing making the path absolute, so `/` (or `D:\\`) fell through to the relative branch and resolved to the project root instead of the ancestor chain — silently, and on both platforms. Normalize first and trim the separator off the result instead, which covers both the filesystem-root case and the trailing separator that readlink can return for a Windows junction.
The canonical paths keyed in the same `FileData` map are already in system separators, so storing `H.SYMLINK` in POSIX made one field of the tuple disagree with its own key, and cost a `replace()` allocation on every traversal of every symlink on Windows — where the old `WeakMap` had done the conversion once per node. The cache is machine-local, so cross-platform identity of the stored value buys nothing outside tests. With the conversion gone, `#resolveSymlinkTargetToNormalPath` is a field read plus an assertion, so inline it into its only caller and drop the unused `canonicalPathOfSymlink` parameter. Also updates the `TreeFS` and `FileMetadata` docblocks, which still described the stored value as the literal output of readlink.
`TreeFS` snapshots are meant to be portable between root directories and operating systems (see the SERIALISATION note in `TreeFS.js`), and a symlink's target is the only path stored inside the tree. Storing it in system separators broke that in both directions: a snapshot built on POSIX no longer followed symlinks on Windows, which works today, and one built on Windows still failed on POSIX. Store the normal target with POSIX separators at both write sites, and convert it to system separators where traversal reads it. This reverses the storage change in bab1a2d and keeps its inlining and docblock updates. The conversion is the identity function on POSIX. On Windows it allocates on each traversal of a symlink, which can be memoized per node on Windows only if it shows up in measurements. Adds a test that builds a snapshot under one platform's `path` module, loads it under the other, and looks up a file through a symlink.
`readlink` returns a target exactly as the link was created, which need not be well-formed. `RootPathUtils`' fast paths require well-formed input, so a symlink to `..` or `b/..` made any lookup through it throw `Failed to collapse`, a symlink to `.` stored a non-normal path, and a target containing `//` was never found. Main handles all of these, because it resolves targets with `path.resolve`. Resolve the target with `path.resolve` relative to the symlink's directory, then normalize it with `absoluteToNormal`. This runs once per symlink, when its node is populated, and made no measurable difference to crawl time on a pnpm install with 3,030 symlinks. The separator conversion before it is dropped, since win32 `path.resolve` accepts `/`.
"Resolved" can be read as a real path. The stored target is only the literal target interpreted against the symlink's directory: symlinks inside it are not followed and it need not exist, so in `TreeFS` terms it is a `normalPath`, not a `canonicalPath`. Say so wherever the `H.SYMLINK` slot is described, including the cache format comment in `index.js`, which still described the stored value as the raw target.
…mlink The guard for adding the root and its collapsed ancestors assigned `getAncestorOfRootIdx(target)` to the traversal's own `ancestorOfRootIdx` state, which was correct only because that variable is reset before the next segment is read. The stored value was never needed: `getAncestorOfRootIdx(x) === 0` holds only for `x === ''`, and the inner `i > 0 || ancestorOfRootIdx === 0 || collapsedSegments > 0` check repeats the guard, so it is always true inside the block. Test the target against `''` in the guard, drop the inner check, and remove `RootPathUtils.getAncestorOfRootIdx`, which has no other callers. Both cases of the guard are covered by existing `TreeFS` tests, and a lookup benchmark on a pnpm install with 3,030 symlinks is unchanged.
A relative target can contain more `..` segments than there are directories above the symlink. `..` at the filesystem root is the root itself, so such a target means the filesystem root, and must be stored as the path from the project root to it rather than going further up. Cover relative targets that reach and pass the filesystem root alongside the existing absolute `/` case, both in `resolveSymlinkToNormal` and in lookups through the stored target.
|
Benchmarked the current head (e6e45d3) against its merge base on Each lookup pass is 10,480 lookups through symlinks (the link itself,
Bold where the CI excludes zero. |
Resolves conflicts with react#1687 (symlink targets stored lexically resolved, as normal POSIX paths) and react#1693 (native `find` crawler removed). - The Node crawler keeps this PR's lazy `lstat` inside main's single `find` path. - Symlinks with a known `mtime` are still read eagerly by `FileMap#maybeReadLink`, which now stores main's format. A symlink crawled without an `lstat` is still `1` until a lookup traverses it, and `TreeFS#readSymlinkTarget` then reads it and stores the target in that same format, so a target read lazily and one read eagerly are indistinguishable. This replaces the `WeakMap`-based `#resolveSymlinkTargetToNormalPath`, which main removed. - The `find` binary tests are dropped along with the crawler they covered, the committed `types/` defs are dropped as on main, and the tests mock `node:fs` to match main's imports.
2 participants
Summary
Note
This is part of the same pick/patch-set from an experiment on the Expo repo,
like #1676, #1677, and #1686, for
metro-file-map(conflicts are expected, since changes were pulled out into separate PRs for clarity)The symlinks have two oddities to them compared to other file data operations and entries:
WeakMapcache is kept for resolved symlink targets that have been convertedThe
WeakMap(in theory) is inefficient especially for projects with a larger amount of symlinks. It's also not necessary since the values can be trivially pre-resolved to a normal path. We can additionally store this value as a posix normal path, to make sure that it's identical between systems, which may help testing.Edit: A primitive (LLM-generated) benchmark that shows that performance remains within margins with this change can be found at kitten@eee7a82
Changelog: [Internal] Pre-resolve symlinks and store normalized symlink targets in file map
Test plan