Improve fast_path.resolve correctness and performance #1209
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
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D52464135 |
robhogan
force-pushed
the
export-D52464135
branch
from
221da10
to
036bb3c
Compare
|
This pull request was exported from Phabricator. Differential Revision: D52464135 |
Summary:
## Correctness
Fixes a bug in `fastPath.resolve` where paths ending in `/..` would not collapse preceding segments, eg `resolve('/project/root', '../..')` would return `/project/..` rather than `/`.
This came up when implementing incremental resolution support - we end up resolving canonical paths like this during lookup of first missing directory nodes. The correctness fix is changelog-internal because Metro doesn't currently call `resolve` with paths like these, and this API isn't public.
## Performance
In implementing this, I've also squeezed out a bit more performance so that `fast_path.resolve` is now about 1.6x faster than before this diff and 20x faster than `path.resolve` for repeated calls with the same root. This is significant because it actually dominates `getRealPath`, which is called hundreds of thousands of times during module resolution on a typical graph. Zoomed out benchmarks to follow in the stack.
Changelog: [Performance] Improve resolution performance by optimising normal->absolute path conversion
## Benchmarks
Calling `resolve` 2m times on different, representative (FB internal) paths per sample.
```
┌─────────┬───────────┬─────────┬───────────────────┬──────────┬─────────┐
│ (index) │ Task Name │ ops/sec │ Average Time (ns) │ Margin │ Samples │
├─────────┼───────────┼─────────┼───────────────────┼──────────┼─────────┤
│ 0 │ 'new' │ '16' │ 61555575.15459959 │ '±0.26%' │ 488 │
│ 1 │ 'old' │ '10' │ 99607323.81785941 │ '±0.32%' │ 302 │
│ 2 │ 'system' │ '0' │ 1472189198.380425 │ '±0.73%' │ 21 │
└─────────┴───────────┴─────────┴───────────────────┴──────────┴─────────┘
```
Reviewed By: motiz88
Differential Revision: D52464135
robhogan
force-pushed
the
export-D52464135
branch
from
036bb3c
to
282ebf2
Compare
|
This pull request was exported from Phabricator. Differential Revision: D52464135 |
|
This pull request has been merged in c2022d4. |
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.
Correctness
Fixes a bug in
fastPath.resolvewhere paths ending in/..would not collapse preceding segments, egresolve('/project/root', '../..')would return/project/..rather than/.This came up when implementing incremental resolution support - we end up resolving canonical paths like this during lookup of first missing directory nodes. The correctness fix is changelog-internal because Metro doesn't currently call
resolvewith paths like these, and this API isn't public.Performance
In implementing this, I've also squeezed out a bit more performance so that
fast_path.resolveis now about 1.6x faster than before this diff and 20x faster thanpath.resolvefor repeated calls with the same root. This is significant because it actually dominatesgetRealPath, which is called hundreds of thousands of times during module resolution on a typical graph. Zoomed out benchmarks to follow in the stack.Benchmarks
Calling
resolve2m times on different, representative (FB internal) paths per sample.Changelog:
Reviewed By: motiz88
Differential Revision: D52464135