FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs,module: add module-loader-only realpath cache by addaleax · Pull Request #8100 · nodejs/node · GitHub

/ node Public

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

Closed
addaleax wants to merge 5 commits into
nodejs:masterfrom
addaleax:fs-module-only-rpcache
ConversationCommits (5)ChecksFiles changed
Closed

fs,module: add module-loader-only realpath cache#8100
addaleax wants to merge 5 commits into
nodejs:masterfrom
addaleax:fs-module-only-rpcache

Conversation

addaleax commented Aug 14, 2016

Copy link
Copy Markdown
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 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 reviewed Aug 14, 2016
View reviewed changes
Comment thread lib/fs.js Outdated

mscdex Aug 14, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

addaleax Aug 14, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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 commented Aug 14, 2016

Copy link
Copy Markdown
Member Author

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

jasnell commented Aug 15, 2016

Copy link
Copy Markdown
Member

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

bzoz commented Aug 16, 2016

Copy link
Copy Markdown
Contributor

LGTM. The CI failure is 99% unrelated, but still - one more run: https://ci.nodejs.org/job/node-test-pull-request/3692/

jasnell commented Aug 18, 2016

Copy link
Copy Markdown
Member

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 reviewed Aug 18, 2016
View reviewed changes
Comment thread lib/module.js Outdated

vkurchatkin Aug 18, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can't this be a const?

addaleax Aug 18, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@vkurchatkin Yes, makes sense. Done!

jasnell commented Aug 18, 2016

Copy link
Copy Markdown
Member

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

jasnell commented Aug 22, 2016

Copy link
Copy Markdown
Member

ping @nodejs/ctc

addaleax commented Aug 27, 2016

Copy link
Copy Markdown
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 commented Sep 9, 2016

Copy link
Copy Markdown
Member Author

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

cjihrig reviewed Sep 10, 2016
View reviewed changes
Comment thread lib/module.js Outdated

cjihrig Sep 10, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should this be "an empty Map"?

addaleax Sep 10, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, thanks for noticing! Done!

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

addaleax commented Sep 10, 2016

Copy link
Copy Markdown
Member Author

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

jasnell commented Sep 12, 2016

Copy link
Copy Markdown
Member

Still LGTM

addaleax commented Sep 25, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 self-assigned this Sep 30, 2016

addaleax commented Sep 30, 2016

Copy link
Copy Markdown
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 commented Sep 30, 2016

Copy link
Copy Markdown
Member Author

Landed in c084287

addaleax closed this Sep 30, 2016
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
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
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
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
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
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
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
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 mentioned this pull request Nov 23, 2016
Closed
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

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

Back | FazBrowse Home | New Git URL