module: skip directories known not to exist by bnoordhuis · Pull Request #9196 · nodejs/node

bnoordhuis · 2016-10-19T21:54:28Z

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

LGTM

bnoordhuis · 2016-10-19T22:06:36Z

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 · 2016-10-19T22:36:27Z

LGTM

evanlucas

LGTM

Fishrock123 · 2016-10-20T10:44:41Z

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 · 2016-10-20T11:05:50Z

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

jasnell · 2016-10-20T17:02:43Z

 assert.notEqual(threeFolder, three);

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


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

I followed the style of the surrounding code here.

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

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

EDIT: #9263

jasnell

LGTM with a nit

Verify that a package.json without a .main property loads index.js. PR-URL: nodejs#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>

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: nodejs#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>

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: nodejs#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>

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>

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>

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 · 2016-11-17T22:42:30Z

@bnoordhuis should this be backported?

bnoordhuis · 2016-11-18T11:00:33Z

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

sam-github · 2017-04-20T14:36:55Z

@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 · 2017-04-20T17:29:03Z

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 · 2017-04-20T17:35:26Z

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

nodejs-github-bot added the module Issues and PRs related to the module subsystem. label Oct 19, 2016

silverwind approved these changes Oct 19, 2016

View reviewed changes

evanlucas approved these changes Oct 19, 2016

View reviewed changes

cjihrig approved these changes Oct 20, 2016

View reviewed changes

targos approved these changes Oct 20, 2016

View reviewed changes

jasnell reviewed Oct 20, 2016

View reviewed changes

jasnell approved these changes Oct 20, 2016

View reviewed changes

bnoordhuis added 3 commits October 24, 2016 22:48

bnoordhuis force-pushed the skip-enoent-module-dirs branch from 8f5dfbc to 678c094 Compare October 24, 2016 20:51

bnoordhuis closed this Oct 24, 2016

bnoordhuis deleted the skip-enoent-module-dirs branch October 24, 2016 20:51

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

evanlucas mentioned this pull request Nov 3, 2016

v7.1.0 proposal - 2016-11-08 #9438

Merged

MylesBorins added lts-watch-v4.x labels Nov 17, 2016

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Mar 9, 2017

MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label May 8, 2017

MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018

Uh oh!

Conversation

bnoordhuis commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverwind left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Oct 19, 2016

Uh oh!

mscdex commented Oct 19, 2016

Uh oh!

evanlucas left a comment

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Oct 20, 2016

Uh oh!

jasnell Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Nov 17, 2016

Uh oh!

bnoordhuis commented Nov 18, 2016

Uh oh!

sam-github commented Apr 20, 2017

Uh oh!

bnoordhuis commented Apr 20, 2017

Uh oh!

MylesBorins commented Apr 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

bnoordhuis commented Oct 19, 2016 •

edited

Loading

Fishrock123 commented Oct 20, 2016 •

edited

Loading

jasnell Oct 20, 2016 •

edited

Loading

bnoordhuis Oct 20, 2016 •

edited

Loading

bnoordhuis Oct 24, 2016 •

edited

Loading