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

Cache non-symlinks in realpathSync by yallop · Pull Request #10253 · nodejs/node · GitHub

/ node Public

Cache non-symlinks in realpathSync - #10253

Closed
yallop wants to merge 1 commit into
nodejs:masterfrom
yallop:cache-non-symlinks
ConversationCommits (1)ChecksFiles changed
Closed

Cache non-symlinks in realpathSync#10253
yallop wants to merge 1 commit into
nodejs:masterfrom
yallop:cache-non-symlinks

Conversation

yallop commented Dec 13, 2016

Copy link
Copy Markdown
Contributor
Background

The fs.realpathSync function synchronously traverses paths componentwise to resolve symbolic links.

The optional cache argument stores the results of resolution to avoid repeated lookups. The cache can be used to override resolution and to store the results of calls to readlinkSync.

However, the cache only records paths that lstat reveals to be symlinks. Consequently, a call to fs.realpathSync for a path without symlinks may result in many calls to lstat, even if most parts of the path have been resolved in an earlier call.

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

Description of change

This tiny patch extends fs.realpathSync to record the results for paths that are not symlinks. The code suggests that this was originally the intention, since the cache is expected to contain entries that resolve to themselves:

cache.get(base) === base

With this patch a run of ember build on a fresh application makes around 6,200 fewer lstat calls (out of a total of around 70,000 syscalls). On a typical native file system the performance difference is negligible, but in situations where a syscall has more significant overhead it can be a worthwhile saving. For example, with a file system that forwards operations over a socket via FUSE this patch saves around 10% of the build time.

Cache non-symlinks in realpathSync.
d3276fd 
Extend `fs.realpathSync` to cache the results for paths that are not
symlinks in addition to caching symlink mappings.

Signed-off-by: Jeremy Yallop <yallop@docker.com>
nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. lts-watch-v6.x labels Dec 13, 2016

mscdex commented Dec 13, 2016

Copy link
Copy Markdown
Contributor

What about fs.realPath()?

addaleax commented Dec 13, 2016

Copy link
Copy Markdown
Member

What about fs.realpath()?

We removed the cache argument in v6. The cache in fs.realpathSync is exlusively used by the module loader now, not part of a public API.

addaleax approved these changes Dec 13, 2016
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

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

LGTM with green CI

CI: https://ci.nodejs.org/job/node-test-commit/6630/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/479/

addaleax added the performance Issues and PRs related to the performance of Node.js. label Dec 13, 2016

yallop commented Dec 14, 2016

Copy link
Copy Markdown
Contributor Author

Thanks for the reviews. I'm not sure why the test/arm CI failed, but let me know if there's anything more I need to do here.

gibfahn commented Dec 23, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

CI 2: https://ci.nodejs.org/job/node-test-commit/6831/
CITGM 2: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/497/

(ARM failures were probably machine issues)

Trott commented Dec 23, 2016

Copy link
Copy Markdown
Member

OS X failure looks like an unrelated race condition in test-http-client-timeout-with-data to me but let's re-run CI to confirm...

CI: https://ci.nodejs.org/job/node-test-pull-request/5561/

Trott commented Dec 23, 2016

Copy link
Copy Markdown
Member

Hmmm, now an AIX failure, also seemingly unrelated. Peculiar.

Let's try again:

CI: https://ci.nodejs.org/job/node-test-pull-request/5562/

Trott commented Dec 23, 2016

Copy link
Copy Markdown
Member

There we go. CI is ✅

jasnell pushed a commit that referenced this pull request Dec 24, 2016
fs: cache non-symlinks in realpathSync.
5dc4487 
Extend `fs.realpathSync` to cache the results for paths that are not
symlinks in addition to caching symlink mappings.

PR-URL: #10253
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell commented Dec 24, 2016

Copy link
Copy Markdown
Member

Landed in 5dc4487

jasnell closed this Dec 24, 2016
yallop deleted the cache-non-symlinks branch December 26, 2016 10:35
evanlucas mentioned this pull request Jan 3, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
fs: cache non-symlinks in realpathSync.
094a960 
Extend `fs.realpathSync` to cache the results for paths that are not
symlinks in addition to caching symlink mappings.

PR-URL: #10253
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
fs: cache non-symlinks in realpathSync.
6629f8f 
Extend `fs.realpathSync` to cache the results for paths that are not
symlinks in addition to caching symlink mappings.

PR-URL: #10253
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins commented Jan 23, 2017

Copy link
Copy Markdown
Contributor

I've gone ahead and backported to v6.x, but am a little bit wary. Is this a good change to backport? It did not land cleanly on v4.x, which makes sense to me. As such I added the appropriate don't land label

MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
fs: cache non-symlinks in realpathSync.
abc7106 
Extend `fs.realpathSync` to cache the results for paths that are not
symlinks in addition to caching symlink mappings.

PR-URL: #10253
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
fs: cache non-symlinks in realpathSync.
f9ff4ec 
Extend `fs.realpathSync` to cache the results for paths that are not
symlinks in addition to caching symlink mappings.

PR-URL: #10253
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins mentioned this pull request Jan 24, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
fs: cache non-symlinks in realpathSync.
709b9b4 
Extend `fs.realpathSync` to cache the results for paths that are not
symlinks in addition to caching symlink mappings.

PR-URL: #10253
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Feb 21, 2017
2017-02-21, Version 6.10.0 'Boron' (LTS)
1b6fd44 
Notable Changes:

The SEMVER-MINOR changes include:

*  crypto: allow adding extra certs to well-known CAs (Sam Roberts)
   #9139
*  deps: Upgrade INTL ICU to version 58 (Steven R. Loomis)
   #9234
*  process: add `process.memoryUsage.external` (Fedor Indutny)
   #9587
*  src: add wrapper for process.emitWarning() (Sam Roberts)
   #9139

Notable SEMVER-PATCH changes include:

*  fs: cache non-symlinks in realpathSync. (Jeremy Yallop)
   #10253
*  repl: allow autocompletion for scoped packages (Evan Lucas)
   #10296
MylesBorins added a commit that referenced this pull request Feb 22, 2017
2017-02-21, Version 6.10.0 'Boron' (LTS)
aecb085 
Notable Changes:

The SEMVER-MINOR changes include:

*  crypto: allow adding extra certs to well-known CAs (Sam Roberts)
   #9139
*  deps: Upgrade INTL ICU to version 58 (Steven R. Loomis)
   #9234
*  process: add `process.memoryUsage.external` (Fedor Indutny)
   #9587
*  src: add wrapper for process.emitWarning() (Sam Roberts)
   #9139

Notable SEMVER-PATCH changes include:

*  fs: cache non-symlinks in realpathSync. (Jeremy Yallop)
   #10253
*  repl: allow autocompletion for scoped packages (Evan Lucas)
   #10296

PR-URL: #10974
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
2017-02-21, Version 6.10.0 'Boron' (LTS)
edbc1bf 
    Notable Changes:

    The SEMVER-MINOR changes include:

    *  crypto: allow adding extra certs to well-known CAs (Sam Roberts)
       nodejs/node#9139
    *  deps: Upgrade INTL ICU to version 58 (Steven R. Loomis)
       nodejs/node#9234
    *  process: add `process.memoryUsage.external` (Fedor Indutny)
       nodejs/node#9587
    *  src: add wrapper for process.emitWarning() (Sam Roberts)
       nodejs/node#9139

    Notable SEMVER-PATCH changes include:

    *  fs: cache non-symlinks in realpathSync. (Jeremy Yallop)
       nodejs/node#10253
    *  repl: allow autocompletion for scoped packages (Evan Lucas)
       nodejs/node#10296

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
2017-02-21, Version 6.10.0 'Boron' (LTS)
f99e583 
    Notable Changes:

    The SEMVER-MINOR changes include:

    *  crypto: allow adding extra certs to well-known CAs (Sam Roberts)
       nodejs/node#9139
    *  deps: Upgrade INTL ICU to version 58 (Steven R. Loomis)
       nodejs/node#9234
    *  process: add `process.memoryUsage.external` (Fedor Indutny)
       nodejs/node#9587
    *  src: add wrapper for process.emitWarning() (Sam Roberts)
       nodejs/node#9139

    Notable SEMVER-PATCH changes include:

    *  fs: cache non-symlinks in realpathSync. (Jeremy Yallop)
       nodejs/node#10253
    *  repl: allow autocompletion for scoped packages (Evan Lucas)
       nodejs/node#10296

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
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

jasnell jasnell approved these changes
addaleax addaleax approved these changes

Assignees

No one assigned

Labels

fs Issues and PRs related to the fs subsystem / file system. performance Issues and PRs related to the performance of Node.js.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL