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

Cache non-symlinks in realpathSync #10253

Closed
wants to merge 1 commit into from
Closed

Cache non-symlinks in realpathSync #10253

wants to merge 1 commit into from

Conversation

yallop
Copy link
Contributor

@yallop yallop commented Dec 13, 2016

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.

@yallop
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 nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. lts-watch-v6.x labels Dec 13, 2016
@mscdex
Copy link
Contributor

mscdex commented Dec 13, 2016

What about fs.realPath()?

@addaleax
Copy link
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
addaleax approved these changes Dec 13, 2016
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

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 addaleax added the performance Issues and PRs related to the performance of Node.js. label Dec 13, 2016
@yallop
Copy link
Contributor Author

yallop commented Dec 14, 2016

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
Copy link
Member

gibfahn commented Dec 23, 2016 •
edited
Loading

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
Copy link
Member

Trott commented Dec 23, 2016

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
Copy link
Member

Trott commented Dec 23, 2016

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
Copy link
Member

Trott commented Dec 23, 2016

There we go. CI is ✅

jasnell pushed a commit that referenced this pull request Dec 24, 2016
@yallop @jasnell
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
Copy link
Member

jasnell commented Dec 24, 2016

Landed in 5dc4487

@jasnell jasnell closed this Dec 24, 2016
@yallop yallop deleted the cache-non-symlinks branch December 26, 2016 10:35
@evanlucas evanlucas mentioned this pull request Jan 3, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
@yallop @evanlucas
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
@yallop @evanlucas
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
Copy link
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
@yallop @MylesBorins
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
@yallop @MylesBorins
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 MylesBorins mentioned this pull request Jan 24, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
@yallop @MylesBorins
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
@MylesBorins
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
@MylesBorins
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
@imyller
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
@imyller
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell 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
@yallop @mscdex @addaleax @gibfahn @Trott @jasnell @MylesBorins @nodejs-github-bot