fs,module: add module-loader-only realpath cache #8100
Closed
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
addaleax
added
fs
| knownHard[base] = true; | ||
| continue; | ||
| } | ||
| if (cache && cache.has(base)) { |
There was a problem hiding this comment.
Would it be faster to just do cache.get() and check if it returned undefined?
There was a problem hiding this comment.
@mscdex Maybe a bit, yes. I’ve updated the PR with only calls to cache.get().
Fishrock123
added
semver-minor
|
LGTM . There's Red in CI that looks unrelated. |
|
LGTM. The CI failure is 99% unrelated, but still - one more run: https://ci.nodejs.org/job/node-test-pull-request/3692/ |
|
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/361/ Because this touches module, I'd like a bit more signoff from @nodejs/ctc |
| // In order to minimize unnecessary lstat() calls, | ||
| // this cache is a list of known-real paths. | ||
| // Set to an empty object to reset. | ||
| Module._realpathCache = new Map(); |
There was a problem hiding this comment.
Can't this be a const?
There was a problem hiding this comment.
@vkurchatkin Yes, makes sense. Done!
|
Marking this semver-minor given that it's technically additive. (I sure will be happy when we have all this fs module stuff settled) |
jasnell
added
the
semver-minor
|
ping @nodejs/ctc |
|
hmm… anybody from @nodejs/ctc @nodejs/collaborators up for more reviews/thoughts on this? This is not really urgent, but having it land in the next couple of days would be nice because it’s in a similar situation as Rod’s #8277, and should get a follow-up PR moving the cache symbol to |
|
hmm… bump again? would anybody feel more comfortable signing off on this if it’s marked |
| @@ -109,6 +109,14 @@ function tryPackage(requestPath, exts, isMain) { | |||
| tryExtensions(path.resolve(filename, 'index'), exts, isMain); | |||
| } | |||
|
|
|||
| // In order to minimize unnecessary lstat() calls, | |||
| // this cache is a list of known-real paths. | |||
| // Set to an empty object to reset. | |||
There was a problem hiding this comment.
Should this be "an empty Map"?
There was a problem hiding this comment.
Yes, thanks for noticing! Done!
Reintroduce a realpath cache with the same mechanisms which existed before b488b19 (`fs: optimize realpath using uv_fs_realpath()`), but only for the synchronous version and with the cache being passed as a hidden option to make sure it is only used internally. The cache is hidden from userland applications because it has been decided that fully reintroducing as part of the public API might stand in the way of future optimizations.
addaleax
force-pushed
the
fs-module-only-rpcache
branch
from
c7efb61
to
c9954f4
Compare
|
Rebased, new CI: https://ci.nodejs.org/job/node-test-commit/4990/ |
|
Still LGTM |
|
Okay, so, unless anybody objects or something else comes up, I’d like to land this sometime next week. Right now it looks like this doesn’t get any more review/feedback than @bzoz’ and @jasnell’s LGTMs. If anybody would feel more comfortable with it, they can add a Edit: One more CI: https://ci.nodejs.org/job/node-test-commit/5375/ |
|
I’ll land this:
|
|
Landed in c084287 |
addaleax
added a commit
that referenced
this pull request
Sep 30, 2016
Reintroduce a realpath cache with the same mechanisms which existed before b488b19 (`fs: optimize realpath using uv_fs_realpath()`), but only for the synchronous version and with the cache being passed as a hidden option to make sure it is only used internally. The cache is hidden from userland applications because it has been decided that fully reintroducing as part of the public API might stand in the way of future optimizations. PR-URL: #8100 Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell
pushed a commit
that referenced
this pull request
Oct 6, 2016
Reintroduce a realpath cache with the same mechanisms which existed before b488b19 (`fs: optimize realpath using uv_fs_realpath()`), but only for the synchronous version and with the cache being passed as a hidden option to make sure it is only used internally. The cache is hidden from userland applications because it has been decided that fully reintroducing as part of the public API might stand in the way of future optimizations. PR-URL: #8100 Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123
pushed a commit
that referenced
this pull request
Oct 11, 2016
Reintroduce a realpath cache with the same mechanisms which existed before b488b19 (`fs: optimize realpath using uv_fs_realpath()`), but only for the synchronous version and with the cache being passed as a hidden option to make sure it is only used internally. The cache is hidden from userland applications because it has been decided that fully reintroducing as part of the public API might stand in the way of future optimizations. PR-URL: #8100 Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123
added a commit
that referenced
this pull request
Oct 12, 2016
* fs: - `SyncWriteStream` now inherits from `Stream.Writable`. (Anna Henningsen) #8830 - Practically, this means that when stdio is piped to a file, stdout and stderr will still be `Writable` streams. - `fs.existsSync()` has been undeprecated. `fs.exists()` remains deprecated. (Dan Fabulich) #8364 * http: `http.request()` now accepts a `timeout` option. (Rene Weber) #8101 * module: The module loader now maintains its own realpath cache. (Anna Henningsen) #8100 * npm: Upgraded to 3.10.8 (Kat Marchán) #8706 * stream: `Duplex` streams now show proper `instanceof Stream.Writable`. (Anna Henningsen) #8834 * timers: Improved `setTimeout`/`Interval` performance by up to 22%. (Brian White) #8661 PR-URL: #9034
Fishrock123
added a commit
that referenced
this pull request
Oct 12, 2016
* fs: - `SyncWriteStream` now inherits from `Stream.Writable`. (Anna Henningsen) #8830 - Practically, this means that when stdio is piped to a file, stdout and stderr will still be `Writable` streams. - `fs.existsSync()` has been undeprecated. `fs.exists()` remains deprecated. (Dan Fabulich) #8364 * http: `http.request()` now accepts a `timeout` option. (Rene Weber) #8101 * module: The module loader now maintains its own realpath cache. (Anna Henningsen) #8100 * npm: Upgraded to 3.10.8 (Kat Marchán) #8706 * stream: `Duplex` streams now show proper `instanceof Stream.Writable`. (Anna Henningsen) #8834 * timers: Improved `setTimeout`/`Interval` performance by up to 22%. (Brian White) #8661 PR-URL: #9034
imyller
added a commit
to imyller/meta-nodejs
that referenced
this pull request
Oct 13, 2016
* fs:
- `SyncWriteStream` now inherits from `Stream.Writable`. (Anna
Henningsen) nodejs/node#8830
- Practically, this means that when stdio is piped to a file,
stdout and stderr will still be `Writable` streams.
- `fs.existsSync()` has been undeprecated. `fs.exists()` remains
deprecated. (Dan Fabulich) nodejs/node#8364
* http: `http.request()` now accepts a `timeout` option. (Rene Weber)
nodejs/node#8101
* module: The module loader now maintains its own realpath cache. (Anna
Henningsen) nodejs/node#8100
* npm: Upgraded to 3.10.8 (Kat Marchan)
nodejs/node#8706
* stream: `Duplex` streams now show proper `instanceof
Stream.Writable`. (Anna Henningsen)
nodejs/node#8834
* timers: Improved `setTimeout`/`Interval` performance by up to 22%.
(Brian White) nodejs/node#8661
Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller
added a commit
to imyller/meta-nodejs
that referenced
this pull request
Oct 13, 2016
* fs:
- `SyncWriteStream` now inherits from `Stream.Writable`. (Anna
Henningsen) nodejs/node#8830
- Practically, this means that when stdio is piped to a file,
stdout and stderr will still be `Writable` streams.
- `fs.existsSync()` has been undeprecated. `fs.exists()` remains
deprecated. (Dan Fabulich) nodejs/node#8364
* http: `http.request()` now accepts a `timeout` option. (Rene Weber)
nodejs/node#8101
* module: The module loader now maintains its own realpath cache. (Anna
Henningsen) nodejs/node#8100
* npm: Upgraded to 3.10.8 (Kat Marchan)
nodejs/node#8706
* stream: `Duplex` streams now show proper `instanceof
Stream.Writable`. (Anna Henningsen)
nodejs/node#8834
* timers: Improved `setTimeout`/`Interval` performance by up to 22%.
(Brian White) nodejs/node#8661
Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
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.
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
fs, module
Description of change
Reintroduce a realpath cache with the same mechanisms which existed before b488b19 (
fs: optimize realpath using uv_fs_realpath()), but only for the synchronous version and with the cache being passed as a hidden option to make sure it is only used internally.The cache is hidden from userland applications because it has been decided that fully reintroducing as part of the public API might stand in the way of future optimizations.
/cc @nodejs/fs @bzoz