fs,module: add module-loader-only realpath cache#8100
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
addaleax added fs 
Fishrock123 added semver-minor 
MemberAuthor
addaleax commented Aug 14, 2016
Member
jasnell commented Aug 15, 2016
LGTM . There's Red in CI that looks unrelated. |
Contributor
bzoz commented Aug 16, 2016
LGTM. The CI failure is 99% unrelated, but still - one more run: https://ci.nodejs.org/job/node-test-pull-request/3692/ |
Member
jasnell commented Aug 18, 2016
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 |
lib/module.js Outdated
Member
jasnell commented Aug 18, 2016
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 Member
jasnell commented Aug 22, 2016
ping @nodejs/ctc |
MemberAuthor
addaleax commented Aug 27, 2016
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 |
MemberAuthor
addaleax commented Sep 9, 2016
hmm… bump again? would anybody feel more comfortable signing off on this if it’s marked |
lib/module.js Outdated
Contributor
There was a problem hiding this comment.
Should this be "an empty Map"?
MemberAuthor
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.
addaleaxforce-pushed the fs-module-only-rpcache branch from c7efb61 to c9954f4CompareMemberAuthor
addaleax commented Sep 10, 2016
Rebased, new CI: https://ci.nodejs.org/job/node-test-commit/4990/ |
Member
jasnell commented Sep 12, 2016
Still LGTM |
MemberAuthor
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/ |
MemberAuthor
addaleax commented Sep 30, 2016
I’ll land this:
|
MemberAuthor
addaleax commented Sep 30, 2016
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
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 <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
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