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.

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

module: skip directories known not to exist #9196

Merged
merged 3 commits into from Oct 24, 2016

Conversation

@bnoordhuis
Copy link
Member

commented Oct 19, 2016

There is no point in trying to search for files in a directory that
we know does not exist, so stop doing that.

Reduces the total number of stat(2) calls and the number of stat(2)
misses on a medium-sized application by about 21% and 29% respectively.

Reduces the total number of package.json open(2) calls and the number
of open(2) misses by about 21% and 93% (!) respectively.

Before:

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 50.93    0.178419          38      4702           lstat
 29.08    0.101875          36      2800      2010 stat
 11.36    0.039796          43       932       215 open
  5.39    0.018897          34       550           fstat
  3.24    0.011337          34       336           pread
------ ----------- ----------- --------- --------- ----------------
100.00    0.350324                  9320      2225 total

After:

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 55.49    0.176638          38      4702           lstat
 24.76    0.078826          35      2225      1435 stat
 10.19    0.032434          44       733        16 open
  6.19    0.019719          36       550           fstat
  3.37    0.010723          32       336           pread
------ ----------- ----------- --------- --------- ----------------
100.00    0.318340                  8546      1451 total

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

@silverwind
Copy link
Contributor

left a comment

LGTM

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2016

Here is what the access pattern looked liked before this pull request. (File paths shortened for brevity.)

29754 stat(".../node_modules/finalhandler", 0x7fff639476d0) = -1 ENOENT (No such file or directory)
29754 stat(".../node_modules/finalhandler.js", 0x7fff63947630) = -1 ENOENT (No such file or directory)
29754 stat(".../node_modules/finalhandler.json", 0x7fff63947630) = -1 ENOENT (No such file or directory)
29754 stat(".../node_modules/finalhandler.node", 0x7fff63947630) = -1 ENOENT (No such file or directory)
29754 open(".../node_modules/finalhandler/package.json", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
29754 stat(".../node_modules/finalhandler/index.js", 0x7fff63947630) = -1 ENOENT (No such file or directory)
29754 stat(".../node_modules/finalhandler/index.json", 0x7fff63947630) = -1 ENOENT (No such file or directory)
29754 stat(".../node_modules/finalhandler/index.node", 0x7fff63947630) = -1 ENOENT (No such file or directory)

Note how the last four are guaranteed to fail because of the first one.

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2016

LGTM

@evanlucas
Copy link
Member

left a comment

LGTM

@targos
targos approved these changes Oct 20, 2016
@Fishrock123

This comment has been minimized.

Copy link
Member

commented Oct 20, 2016

Does attempting to require from a directory, failing and making a new directory, writing into, and then loading files from that directory still work?

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2016

You say 'still' but does it work now? The stat cache persists for the duration of a module's top-level code.

@@ -69,6 +69,8 @@ assert.equal(threeFolder, threeIndex);
assert.notEqual(threeFolder, three);

console.error('test package.json require() loading');
assert.equal(require('../fixtures/packages/index').ok, 'ok',

This comment has been minimized.

Copy link
@jasnell

jasnell Oct 20, 2016

Member

strictEqual?
I know equal is consistent with the rest of the test but updating the test may be worthwhile

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Oct 20, 2016

Author Member

I followed the style of the surrounding code here.

EDIT: Never mind, didn't see you edited your comment. Still, consistency is important, right?

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Oct 24, 2016

Author Member

I'll update the test en bloc in a separate pull request.

EDIT: #9263

@jasnell
Copy link
Member

left a comment

LGTM with a nit

bnoordhuis added 3 commits Oct 19, 2016
test: add more module loader test coverage
Verify that a package.json without a .main property loads index.js.

PR-URL: #9196
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
doc: add performance warning to require.extensions
Discourage using require.extensions because it slows down the module
loader.  The number of file system operations that the module system
has to perform in order to resolve a `require(...)` statement to a
filename is proportional to the number of registered extensions.

PR-URL: #9196
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
module: skip directories known not to exist
There is no point in trying to search for files in a directory that
we know does not exist, so stop doing that.

Reduces the total number of stat(2) calls and the number of stat(2)
misses on a medium-sized application by about 21% and 29% respectively.

Reduces the total number of package.json open(2) calls and the number
of open(2) misses by about 21% and 93% (!) respectively.

Before:

    % time     seconds  usecs/call     calls    errors syscall
    ------ ----------- ----------- --------- --------- ----------------
     50.93    0.178419          38      4702           lstat
     29.08    0.101875          36      2800      2010 stat
     11.36    0.039796          43       932       215 open
      5.39    0.018897          34       550           fstat
      3.24    0.011337          34       336           pread
    ------ ----------- ----------- --------- --------- ----------------
    100.00    0.350324                  9320      2225 total

After:

    % time     seconds  usecs/call     calls    errors syscall
    ------ ----------- ----------- --------- --------- ----------------
     55.49    0.176638          38      4702           lstat
     24.76    0.078826          35      2225      1435 stat
     10.19    0.032434          44       733        16 open
      6.19    0.019719          36       550           fstat
      3.37    0.010723          32       336           pread
    ------ ----------- ----------- --------- --------- ----------------
    100.00    0.318340                  8546      1451 total

PR-URL: #9196
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@bnoordhuis bnoordhuis force-pushed the bnoordhuis:skip-enoent-module-dirs branch from 8f5dfbc to 678c094 Oct 24, 2016

@bnoordhuis bnoordhuis closed this Oct 24, 2016

@bnoordhuis bnoordhuis deleted the bnoordhuis:skip-enoent-module-dirs branch Oct 24, 2016

@bnoordhuis bnoordhuis merged commit 678c094 into nodejs:master Oct 24, 2016

evanlucas added a commit that referenced this pull request Nov 2, 2016
test: add more module loader test coverage
Verify that a package.json without a .main property loads index.js.

PR-URL: #9196
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request Nov 2, 2016
module: skip directories known not to exist
There is no point in trying to search for files in a directory that
we know does not exist, so stop doing that.

Reduces the total number of stat(2) calls and the number of stat(2)
misses on a medium-sized application by about 21% and 29% respectively.

Reduces the total number of package.json open(2) calls and the number
of open(2) misses by about 21% and 93% (!) respectively.

Before:

    % time     seconds  usecs/call     calls    errors syscall
    ------ ----------- ----------- --------- --------- ----------------
     50.93    0.178419          38      4702           lstat
     29.08    0.101875          36      2800      2010 stat
     11.36    0.039796          43       932       215 open
      5.39    0.018897          34       550           fstat
      3.24    0.011337          34       336           pread
    ------ ----------- ----------- --------- --------- ----------------
    100.00    0.350324                  9320      2225 total

After:

    % time     seconds  usecs/call     calls    errors syscall
    ------ ----------- ----------- --------- --------- ----------------
     55.49    0.176638          38      4702           lstat
     24.76    0.078826          35      2225      1435 stat
     10.19    0.032434          44       733        16 open
      6.19    0.019719          36       550           fstat
      3.37    0.010723          32       336           pread
    ------ ----------- ----------- --------- --------- ----------------
    100.00    0.318340                  8546      1451 total

PR-URL: #9196
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas added a commit that referenced this pull request Nov 2, 2016
doc: add performance warning to require.extensions
Discourage using require.extensions because it slows down the module
loader.  The number of file system operations that the module system
has to perform in order to resolve a `require(...)` statement to a
filename is proportional to the number of registered extensions.

PR-URL: #9196
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Nov 17, 2016

@bnoordhuis should this be backported?

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2016

It could but it doesn't have to be. It's an optimization, not a bug fix.

@sam-github

This comment has been minimized.

Copy link
Member

commented Apr 20, 2017

@bnoordhuis @MylesBorins should be labelled either dont-land-on-v6.x or backport-requested-to-v6.x, what say you? I think making v6.x faster is good, but not essential.

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2017

It should be a zero-risk back-port but then again, no one complained it's slow. The most compelling reason would be to avoid future merge conflicts but OTOH, lib/module.js doesn't see all that many updates so probably not much of an issue. /stream-of-consciousness

Don't bother, it's fine.

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Apr 20, 2017

I think we should just keep this labelled and be ready to backport if we run into problems in the future... would prefer not to touch module

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.