Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fs,module: add module-loader-only realpath cache #8100

Closed
wants to merge 5 commits into from
Closed

fs,module: add module-loader-only realpath cache #8100

wants to merge 5 commits into from

Conversation

addaleax
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected 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

@addaleax addaleax added fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem. labels Aug 14, 2016
mscdex
mscdex reviewed Aug 14, 2016
View reviewed changes
lib/fs.js
knownHard[base] = true;
continue;
}
if (cache && cache.has(base)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be faster to just do cache.get() and check if it returned undefined?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mscdex Maybe a bit, yes. I’ve updated the PR with only calls to cache.get().

@Fishrock123 Fishrock123 added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Aug 14, 2016
@addaleax
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/4567/

@jasnell
Copy link
Member

jasnell commented Aug 15, 2016

LGTM . There's Red in CI that looks unrelated.

@bzoz
Copy link
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/

@jasnell
Copy link
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

vkurchatkin
vkurchatkin reviewed Aug 18, 2016
View reviewed changes
lib/module.js
// 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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this be a const?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vkurchatkin Yes, makes sense. Done!

@jasnell
Copy link
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 jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 18, 2016
@jasnell
Copy link
Member

jasnell commented Aug 22, 2016

ping @nodejs/ctc

@addaleax
Copy link
Member Author

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 internal/fs.

@addaleax
Copy link
Member Author

addaleax commented Sep 9, 2016

hmm… bump again? would anybody feel more comfortable signing off on this if it’s marked dont-land-on-v6.x?

cjihrig
cjihrig reviewed Sep 10, 2016
View reviewed changes
lib/module.js
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be "an empty Map"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks for noticing! Done!

@addaleax
fs,module: add module-loader-only realpath cache
020ba8b 
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
Replace cache.has with .get() & check.
434424f
@addaleax
Use local const instead of property on Module object
7f0c224
@addaleax
Address review comments
f3e8373
@addaleax
fixup: id cannot be const
c9954f4
@addaleax addaleax force-pushed the fs-module-only-rpcache branch from c7efb61 to c9954f4 Compare September 10, 2016 10:20
@addaleax
Copy link
Member Author

Rebased, new CI: https://ci.nodejs.org/job/node-test-commit/4990/

@jasnell
Copy link
Member

jasnell commented Sep 12, 2016

Still LGTM

@addaleax
Copy link
Member Author

addaleax commented Sep 25, 2016 •
edited
Loading

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 dont-land-on-v6.x label.

Edit: One more CI: https://ci.nodejs.org/job/node-test-commit/5375/

@addaleax addaleax self-assigned this Sep 30, 2016
@addaleax
Copy link
Member Author

I’ll land this:

  • Two LGTMs
  • No objections or indications of further review
  • Last two CIs add up as green, and the last CI was green up to a build failure and unrelated flaky tests.

@addaleax
Copy link
Member Author

Landed in c084287

@addaleax addaleax closed this Sep 30, 2016
@addaleax addaleax deleted the fs-module-only-rpcache branch September 30, 2016 14:11
addaleax added a commit that referenced this pull request Sep 30, 2016
@addaleax
fs,module: add module-loader-only realpath cache
c084287 
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
@addaleax @jasnell
fs,module: add module-loader-only realpath cache
e75b445 
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
@addaleax @Fishrock123
fs,module: add module-loader-only realpath cache
980c1ed 
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
@Fishrock123
2016-10-12, Version 6.8.0 (Current)
17145bd 
* 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
@Fishrock123
2016-10-12, Version 6.8.0 (Current)
e4ee09a 
* 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
@imyller
2016-10-12, Version 6.8.0 (Current)
c1a0496 
    * 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
@imyller
2016-10-12, Version 6.8.0 (Current)
e6d3406 
    * 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>
@CurryKitten CurryKitten mentioned this pull request Nov 23, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@addaleax addaleax

Labels
fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@addaleax @jasnell @bzoz @mscdex @Fishrock123 @cjihrig @vkurchatkin