Speed up require(), phase 1 and 2 #1801

bnoordhuis · 2015-05-26T19:35:58Z

Replace calls to fs.statSync() with an internal variant that does not
create Error or Stat objects that put strain on the garbage collector.

Replace calls to fs.readFileSync() with an internal variant that does
not create Error objects on failure and is a bit speedier in general.

A secondary benefit is that it improves start-up times in the debugger
because it no longer emits thousands of exception debug events.

On a medium-sized application[0], this commit and its predecessor reduce
start-up times from about 1.5s to 0.5s and reduce the number of start-up
exceptions from ~6100 to 32, half of them internal to the application.

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/712/

R=@trevnorris?

bnoordhuis · 2015-05-26T19:37:19Z

lib/module.js

-    var json = fs.readFileSync(jsonPath, 'utf8');
-  } catch (e) {
+  var jsonPath = path.resolve(requestPath, 'package.json');
+  var json = internalModuleReadFile(jsonPath);


There are two other places where fs.readFileSync could be replaced but that would alter the stack trace in case of error.

I hope and assume no one writes code that depends on the exact stack trace but I decided to let it be for now, just in case.

I don't think that should be a concern, especially not when there's potential perf gains. Nobody should depend on the exact stack trace.

I don't really disagree but the other two call sites don't seem to be bottlenecks like the package.json load step is.

ofrobots · 2015-05-26T20:39:42Z

For posterity, can you provide the missing link for "medium-sized application[0]"?

trevnorris · 2015-05-26T20:48:09Z

src/node_file.cc

+    }
+  }
+
+  fclose(stream);


Totally ridiculous case, but what would happen if the file were deleted while being read? I assume fread() would simply return 1, then fclose() would return EBADF?

Likelihood of this happening is near zero.

On UNIX systems, unlinking the file doesn't delete it until the last file descriptor is closed. If another process truncates it, fwrite() simply returns with a small (possibly zero) read count.

@bnoordhuis
And if the fs gets unmounted or the connection the the remote fs gets broken?
Probablilty = 0, but still?

This PR won't change the observable behavior of require(), if that is what you're asking.

EDIT: Oh wait, I think I get you now. Yes, fclose() can return an error.

trevnorris · 2015-05-26T20:53:45Z

Just a couple questions, but LGTM.

bnoordhuis · 2015-05-26T20:56:10Z

@ofrobots Sorry, it's in the commit log but I forgot to paste it into the description. It's https://github.com/strongloop/loopback-sample-app

bnoordhuis · 2015-05-27T15:28:58Z

Incorporated feedback, PTAL.

trevnorris · 2015-05-27T16:11:33Z

LGTM

Replace calls to fs.statSync() with an internal variant that does not create Error or Stat objects that put strain on the garbage collector. A secondary benefit is that it improves start-up times in the debugger because it no longer emits thousands of exception debug events. PR-URL: nodejs#1801 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Replace calls to fs.readFileSync() with an internal variant that does not create Error objects on failure and is a bit speedier in general. A secondary benefit is that it improves start-up times in the debugger because it no longer emits thousands of exception debug events. On a medium-sized application[0], this commit and its predecessor reduce start-up times from about 1.5s to 0.5s and reduce the number of start-up exceptions from ~6100 to 32, half of them internal to the application. [0] https://github.com/strongloop/loopback-sample-app PR-URL: nodejs#1801 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

ghost · 2015-05-29T06:53:46Z

Woh ,iojs just got faster?

Notable Changes: * node: Speed-up require() by replacing usage of fs.statSync() and fs.readFileSync() with internal variants that are faster for this use-case and do not create as many objects for the garbage collector to clean up. The primary two benefits are: significant increase in application start-up time on typical applications and better start-up time for the debugger by eliminating almost all of the thousands of exception events. (Ben Noordhuis) nodejs#1801. * node: Resolution of pre-load modules (-r or --require) now follows the standard require() rules rather than just resolving paths, so you can now pre-load modules in node_modules. (Ali Ijaz Sheikh) nodejs#1812. * npm: Upgraded npm to v2.11.0. New hooks for preversion, version, and postversion lifecycle events, some SPDX-related license changes and license file inclusions. See the release notes for full details.

PR-URL: nodejs#1808 Notable Changes: * node: Speed-up require() by replacing usage of fs.statSync() and fs.readFileSync() with internal variants that are faster for this use-case and do not create as many objects for the garbage collector to clean up. The primary two benefits are: significant increase in application start-up time on typical applications and better start-up time for the debugger by eliminating almost all of the thousands of exception events. (Ben Noordhuis) nodejs#1801. * node: Resolution of pre-load modules (-r or --require) now follows the standard require() rules rather than just resolving paths, so you can now pre-load modules in node_modules. (Ali Ijaz Sheikh) nodejs#1812. * npm: Upgraded npm to v2.11.0. New hooks for preversion, version, and postversion lifecycle events, some SPDX-related license changes and license file inclusions. See the release notes for full details.

Replace calls to fs.statSync() with an internal variant that does not create Error or Stat objects that put strain on the garbage collector. A secondary benefit is that it improves start-up times in the debugger because it no longer emits thousands of exception debug events. PR-URL: nodejs/node#1801 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Replace calls to fs.readFileSync() with an internal variant that does not create Error objects on failure and is a bit speedier in general. A secondary benefit is that it improves start-up times in the debugger because it no longer emits thousands of exception debug events. On a medium-sized application[0], this commit and its predecessor reduce start-up times from about 1.5s to 0.5s and reduce the number of start-up exceptions from ~6100 to 32, half of them internal to the application. [0] https://github.com/strongloop/loopback-sample-app PR-URL: nodejs/node#1801 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

ChALkeR · 2015-06-16T03:25:16Z

It looks stat at least one module out there was monkey-patching fs and relied on require using that: tschaub/mock-fs#43

But I guess monkey-patching was never supported, because supporting it means freezing everything.

nodejs#1801 introduced internal fs methods to speed up require. The methods do not call path._makeLong like their counterpart from the fs module. This brings back the old behaviour. Fixes: nodejs#1990 Fixes: nodejs#1980 Fixes: nodejs#1849

#1801 introduced internal fs methods to speed up require. The methods do not call path._makeLong like their counterpart from the fs module. This brings back the old behaviour. Fixes: #1990 Fixes: #1980 Fixes: #1849 PR-URL: https://github.com/nodejs/io.js/pull/1991/files Reviewed-By: Bert Belder <bertbelder@gmail.com>

PR-URL: #1996 Notable changes * module: The number of syscalls made during a require() have been significantly reduced again (see #1801 from v2.2.0 for previous work), which should lead to a performance improvement (Pierre Inglebert) #1920. * npm: - Upgrade to v2.11.2 (Rebecca Turner) #1956. - Upgrade to v2.11.3 (Forrest L Norvell) #2018. * zlib: A bug was discovered where the process would abort if the final part of a zlib decompression results in a buffer that would exceed the maximum length of 0x3fffffff bytes (~1GiB). This was likely to only occur during buffered decompression (rather than streaming). This is now fixed and will instead result in a thrown RangeError (Michaël Zasso) #1811.

bnoordhuis reviewed May 26, 2015
View reviewed changes

Fishrock123 added the module Issues and PRs related to the module subsystem. label May 26, 2015

trevnorris reviewed May 26, 2015
View reviewed changes

bnoordhuis force-pushed the speed-up-require branch from dedb944 to 6bf70d5 Compare May 27, 2015 15:28

bnoordhuis added 2 commits May 27, 2015 21:21

bnoordhuis force-pushed the speed-up-require branch from 6bf70d5 to 1bbf8d0 Compare May 27, 2015 19:22

bnoordhuis closed this May 27, 2015

bnoordhuis deleted the speed-up-require branch May 27, 2015 19:22

bnoordhuis merged commit 1bbf8d0 into nodejs:master May 27, 2015

rvagg mentioned this pull request May 30, 2015

Release proposal: v2.2.0 #1808

Merged

blond mentioned this pull request Jun 1, 2015

Require error with io.js@2.2.x tschaub/mock-fs#43

Closed

Fishrock123 mentioned this pull request Jun 9, 2015

Reduce syscalls on require() #1920

Closed

ChALkeR mentioned this pull request Jun 15, 2015

Gulp dependencies not found #1980

Closed

ChALkeR mentioned this pull request Jun 16, 2015

Handling EMFILE/ENFILE on fs.open #1941

Closed

targos mentioned this pull request Jun 16, 2015

lib: allow long paths for require on Windows #1991

Closed

v12 mentioned this pull request Jun 23, 2015

Install fails because module 'is-property' isn't installed electron-userland/electron-prebuilt#40

Closed

ChALkeR added the performance Issues and PRs related to the performance of Node.js. label Feb 16, 2016

qfox mentioned this pull request Feb 17, 2016

Feature request: A way to mock module, require #5290

Closed

bnoordhuis mentioned this pull request Apr 22, 2016

Improve stack traces for exceptions thrown at a top-level #6271

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up require(), phase 1 and 2 #1801

Speed up require(), phase 1 and 2 #1801

bnoordhuis commented May 26, 2015

bnoordhuis May 26, 2015

sindresorhus May 27, 2015

bnoordhuis May 27, 2015

ofrobots commented May 26, 2015

trevnorris May 26, 2015

bnoordhuis May 26, 2015

ChALkeR May 27, 2015

bnoordhuis May 27, 2015

trevnorris commented May 26, 2015

bnoordhuis commented May 26, 2015

bnoordhuis commented May 27, 2015

trevnorris commented May 27, 2015

ghost commented May 29, 2015

ChALkeR commented Jun 16, 2015

Speed up require(), phase 1 and 2 #1801

Speed up require(), phase 1 and 2 #1801

Conversation

bnoordhuis commented May 26, 2015

bnoordhuis May 26, 2015

Choose a reason for hiding this comment

sindresorhus May 27, 2015

Choose a reason for hiding this comment

bnoordhuis May 27, 2015

Choose a reason for hiding this comment

ofrobots commented May 26, 2015

trevnorris May 26, 2015

Choose a reason for hiding this comment

bnoordhuis May 26, 2015

Choose a reason for hiding this comment

ChALkeR May 27, 2015

Choose a reason for hiding this comment

bnoordhuis May 27, 2015

Choose a reason for hiding this comment

trevnorris commented May 26, 2015

bnoordhuis commented May 26, 2015

bnoordhuis commented May 27, 2015

trevnorris commented May 27, 2015

ghost commented May 29, 2015

ChALkeR commented Jun 16, 2015