Resolver perf: Use directory existence to cut down candidate list (3/n) #1333
Conversation
This file contains 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
Summary:
Further optimise resolution with some work avoidance by using the fact that, when resolving `require('foo')`, we can eliminate any candidates of the form `<prefix>/node_modules/foo<suffix>` unless `<prefix>/node_modules` exists and is a directory. This saves a lot of lookups from deeply-nested origin modules.
Further, when resolving `require('foo/bar')`, we can eliminate candidates of the form `<prefix>/node_modules/foo/<suffix>` unless `<prefix>/node_modules/foo` exists as a directory.
This is a bit more subtle than it might seem, and can't be extended beyond these cases, due to redirection. For `require('foo/bar/baz')`, `foo/bar` need not exist anywhere, because some `foo/package.json` could redirect it. However, when looking up a `package.json` for possible redirections, we stop at `node_modules` (we do not check `node_modules/package.json`), so it's safe to treat anything up to and including `node_modules` (or `node_modules/foo` in the case `node_modules/foo.js` may not be a candidate) as a real file path that must be an extant directory.
This fix reduces the existence checks we need to perform in RNTester such that total resolution speed is improved ~3x, or ~12x relative to the start of this stack / the previous Metro release.
```
- **[Performance]**: Use directory existence to cut down resolution candidates
```
Reviewed By: huntie
Differential Revision: D58594993
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D58594993 |
|
This pull request has been merged in 6f3af82. |
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.
Summary:
Further optimise resolution with some work avoidance by using the fact that, when resolving
require('foo'), we can eliminate any candidates of the form<prefix>/node_modules/foo<suffix>unless<prefix>/node_modulesexists and is a directory. This saves a lot of lookups from deeply-nested origin modules.Further, when resolving
require('foo/bar'), we can eliminate candidates of the form<prefix>/node_modules/foo/<suffix>unless<prefix>/node_modules/fooexists as a directory.This is a bit more subtle than it might seem, and can't be extended beyond these cases, due to redirection. For
require('foo/bar/baz'),foo/barneed not exist anywhere, because somefoo/package.jsoncould redirect it. However, when looking up apackage.jsonfor possible redirections, we stop atnode_modules(we do not checknode_modules/package.json), so it's safe to treat anything up to and includingnode_modules(ornode_modules/fooin the casenode_modules/foo.jsmay not be a candidate) as a real file path that must be an extant directory.This fix reduces the existence checks we need to perform in RNTester such that total resolution speed is improved ~3x, or ~12x relative to the start of this stack / the previous Metro release.
Reviewed By: huntie
Differential Revision: D58594993