lib: improve module loading performance #5172

mscdex · 2016-02-10T09:18:00Z

This commit improves module loading performance by at least ~17-23% in the module-loader benchmarks.

This improvement is on top of the module loading performance boost brought on by #5123. Both PRs combined bring a total of ~54% improvement in module loading.

Some optimization strategies include:

Try-finally/try-catch isolation
Replacing regular expressions with manual parsers
Avoiding unnecessary string and array creation
Avoiding constant recompilation of anonymous functions and
function definitions within functions

mscdex · 2016-02-10T09:18:49Z

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

rvagg · 2016-02-10T09:33:48Z

lib/fs.js

+  }
+  return st;
+}
+function tryCreateBuffer(size, fd, isUserFd) {


some spacing between functions would be nice

rvagg · 2016-02-10T09:38:23Z

lib/fs.js

+  var i = start;
+  for (; i < path.length; ++i) {
+    const code = path.charCodeAt(i);
+    if (code === 47/*/*/ || code === 92/*\*/)


can you quote and space these so it doesn't just look like weird comments?
if (code === 47 /* '/' */ || code === 92 /* '\' */)
or
if (code === 47 || code === 92) // '/' or '\'
I personally find the latter clearest

I was mainly keeping things consistent with the path perf PR. I don't really have a strong preference either way, but I do think that having the literals next to the character codes is easier to scan.

I have to agree with @rvagg that the current code comment is rather unreadable (makes my eyes spin ;)) And Rod's latter example is the nicest alternative imho too.

rvagg · 2016-02-10T09:42:15Z

Pretty impressive work, I find the regex replacement with manual parsers the most confronting part of this and worry about its maintainability and approachability. Any idea what portion of the speedup is due to those bits?

If the functions are a nearly direct translation of the regexes then maybe put the regex at the top in a comment to show what it's trying to do.

mscdex · 2016-02-10T09:49:23Z

I don't recall the actual numbers for each regexp removal, but they are a decent amount.

The added path root extraction functions were mostly ripped from the new path changes and I thought about adding them as new path methods, but wasn't so sure about it.

jasnell · 2016-02-10T16:52:53Z

LGTM but I do agree with the concerns over maintainability. Hopefully that's not going to be too much of an issue tho given that this code is pretty stable.

abenhamdine · 2016-02-10T20:21:13Z

lib/module.js


  // For each path
-  for (var i = 0, PL = paths.length; i < PL; i++) {
+  for (var i = 0; i < paths.length; i++) {


Any reason to not use let here instead of var ?

let apparently still has some optimization issues in V8. For hot code paths var is still preferred

Yes, what @jasnell said.

mscdex · 2016-02-19T18:12:09Z

FWIW merging these changes with #3594 actually provides a much larger increase (since less time is spent doing path resolution): ~34% for the full path case and ~25% for the directory case.

jasnell · 2016-02-19T23:26:07Z

Cautiously putting a don't land on v4.x label on. Would want this to sit in a stable for a while before pulling back

ronkorving · 2016-02-27T06:09:54Z

Is anything blocking this from landing? (besides the merge conflict, cc @mscdex)
If I had to guess, the big regex replacement, similar to #5256 is something worth debating (but then let's have that debate).

mscdex · 2016-02-27T20:20:38Z

@ronkorving As soon as the realpath/realpathSync PR (#3594) (and its accompanying libuv update) get merged, the entire custom userland implementation of those methods goes away.

jasnell · 2016-03-21T22:36:35Z

@mscdex ... ping... where is this one at?

mscdex · 2016-03-21T22:39:54Z

@jasnell Well, when both the next version of libuv and #3594 lands, the realpath-related changes can be removed. I'm waiting on those changes to happen first.

jasnell · 2016-03-21T23:01:04Z

sounds good!

mscdex · 2016-04-07T15:26:55Z

I've rebased and removed the realpath changes in anticipation of #3594.

jasnell · 2016-04-07T17:16:52Z

Still LGTM if CI is green

mscdex · 2016-04-14T17:55:33Z

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

PR-URL: nodejs#5172 Reviewed-By: James M Snell <[email protected]>

This commit improves module loading performance by at least ~25-35% in the module-loader benchmarks. Some optimization strategies include: * Try-finally/try-catch isolation * Replacing regular expressions with manual parsing * Avoiding unnecessary string and array creation * Avoiding constant recompilation of anonymous functions and function definitions within functions PR-URL: nodejs#5172 Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2016-04-14T21:31:41Z

@mscdex did you use the merge button for this???? if so I'm glad to see it is working as expected

mscdex · 2016-04-14T22:38:34Z

@thealphanerd No, I didn't use the merge button.

MylesBorins · 2016-04-15T03:42:23Z

@mscdex never mind then... was thrown off by the merge message above. Thanks for the heads up

mhdawson · 2016-04-15T14:46:21Z

Benchmark graph at benchmarking.nodejs.org shows a bump from 14603 to 24127 for the "new" case, nice !

MylesBorins · 2016-04-19T13:43:57Z

Should we land this on v5.x? There are conflicts and I'm imagining we may not want to land something that touches module and may have regressions if we are not cutting another v5.x

mscdex · 2016-04-19T13:47:22Z

@thealphanerd Something else to keep in mind is that the jump you see in that graph is also including the realpath changes, so it's not just this PR. I would probably hold off on backporting to v5.x just to be safe for now.

MylesBorins · 2016-04-19T13:49:59Z

added dont-land thanks fo the quick response

PR-URL: #5172 Reviewed-By: James M Snell <[email protected]>

This commit improves module loading performance by at least ~25-35% in the module-loader benchmarks. Some optimization strategies include: * Try-finally/try-catch isolation * Replacing regular expressions with manual parsing * Avoiding unnecessary string and array creation * Avoiding constant recompilation of anonymous functions and function definitions within functions PR-URL: #5172 Reviewed-By: James M Snell <[email protected]>

This commit improves module loading performance by at least ~25-35% in the module-loader benchmarks. Some optimization strategies include: * Try-finally/try-catch isolation * Replacing regular expressions with manual parsing * Avoiding unnecessary string and array creation * Avoiding constant recompilation of anonymous functions and function definitions within functions PR-URL: nodejs/node#5172 Reviewed-By: James M Snell <[email protected]>

mscdex added the module Issues and PRs related to the module subsystem. label Feb 10, 2016

rvagg reviewed Feb 10, 2016
View reviewed changes

mscdex force-pushed the perf-module-loading branch from cf164c5 to dc56390 Compare February 10, 2016 09:36

rvagg reviewed Feb 10, 2016
View reviewed changes

abenhamdine reviewed Feb 10, 2016
View reviewed changes

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

jasnell added the dont-land-on-v4.x label Feb 19, 2016

mscdex added this to the 6.0.0 milestone Apr 6, 2016

mscdex force-pushed the perf-module-loading branch from dc56390 to f6067f3 Compare April 7, 2016 15:16

mscdex force-pushed the perf-module-loading branch from f6067f3 to ae9783d Compare April 14, 2016 18:55

mscdex added 2 commits April 14, 2016 14:56

benchmark: add module loader benchmark parameter

1e4674a

PR-URL: nodejs#5172 Reviewed-By: James M Snell <[email protected]>

mscdex force-pushed the perf-module-loading branch from ae9783d to ae18bbe Compare April 14, 2016 18:59

mscdex merged commit ae18bbe into nodejs:master Apr 14, 2016

mscdex deleted the perf-module-loading branch April 14, 2016 19:11

MylesBorins added the dont-land-on-v5.x label Apr 19, 2016

MylesBorins pushed a commit that referenced this pull request Apr 19, 2016

benchmark: add module loader benchmark parameter

7503020

PR-URL: #5172 Reviewed-By: James M Snell <[email protected]>

MylesBorins pushed a commit that referenced this pull request Apr 20, 2016

benchmark: add module loader benchmark parameter

ac9a629

PR-URL: #5172 Reviewed-By: James M Snell <[email protected]>

MylesBorins pushed a commit that referenced this pull request Apr 20, 2016

benchmark: add module loader benchmark parameter

39f887e

PR-URL: #5172 Reviewed-By: James M Snell <[email protected]>

MylesBorins mentioned this pull request Apr 20, 2016

V5.10.2 proposal #6311

Closed

MylesBorins pushed a commit that referenced this pull request Apr 20, 2016

benchmark: add module loader benchmark parameter

332f738

PR-URL: #5172 Reviewed-By: James M Snell <[email protected]>

This was referenced Apr 21, 2016

V5.11.0 proposal #6321

Closed

V5.11.0 proposal #6322

Closed

MylesBorins pushed a commit that referenced this pull request Apr 21, 2016

benchmark: add module loader benchmark parameter

5807f9d

PR-URL: #5172 Reviewed-By: James M Snell <[email protected]>

jasnell pushed a commit that referenced this pull request Apr 26, 2016

benchmark: add module loader benchmark parameter

4ea792a

PR-URL: #5172 Reviewed-By: James M Snell <[email protected]>

kevinsawicki mentioned this pull request Jun 23, 2016

Reuse node's implementation of Module._nodeModulePaths electron/electron#6213

Merged

3 tasks

mscdex mentioned this pull request Jan 27, 2017

Update module.js #11032

Closed

4 tasks

MoritzKn mentioned this pull request Apr 1, 2017

Startup Performance: Why are we compiling modules all the time? atom/atom#6593

Closed

oliviertassinari mentioned this pull request Feb 16, 2018

docs: why we use try-finally over try-catch #18711

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: improve module loading performance #5172

lib: improve module loading performance #5172

mscdex commented Feb 10, 2016

mscdex commented Feb 10, 2016

rvagg Feb 10, 2016

mscdex Feb 10, 2016

rvagg Feb 10, 2016

mscdex Feb 10, 2016

ronkorving Feb 27, 2016

rvagg commented Feb 10, 2016

mscdex commented Feb 10, 2016

jasnell commented Feb 10, 2016

abenhamdine Feb 10, 2016

jasnell Feb 10, 2016

mscdex Feb 10, 2016

mscdex commented Feb 19, 2016

jasnell commented Feb 19, 2016

ronkorving commented Feb 27, 2016

mscdex commented Feb 27, 2016

jasnell commented Mar 21, 2016

mscdex commented Mar 21, 2016

jasnell commented Mar 21, 2016

mscdex commented Apr 7, 2016

jasnell commented Apr 7, 2016

mscdex commented Apr 14, 2016

MylesBorins commented Apr 14, 2016

mscdex commented Apr 14, 2016

MylesBorins commented Apr 15, 2016

mhdawson commented Apr 15, 2016

MylesBorins commented Apr 19, 2016

mscdex commented Apr 19, 2016

MylesBorins commented Apr 19, 2016

lib: improve module loading performance #5172

lib: improve module loading performance #5172

Conversation

mscdex commented Feb 10, 2016

mscdex commented Feb 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg commented Feb 10, 2016

mscdex commented Feb 10, 2016

jasnell commented Feb 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Feb 19, 2016

jasnell commented Feb 19, 2016

ronkorving commented Feb 27, 2016

mscdex commented Feb 27, 2016

jasnell commented Mar 21, 2016

mscdex commented Mar 21, 2016

jasnell commented Mar 21, 2016

mscdex commented Apr 7, 2016

jasnell commented Apr 7, 2016

mscdex commented Apr 14, 2016

MylesBorins commented Apr 14, 2016

mscdex commented Apr 14, 2016

MylesBorins commented Apr 15, 2016

mhdawson commented Apr 15, 2016

MylesBorins commented Apr 19, 2016

mscdex commented Apr 19, 2016

MylesBorins commented Apr 19, 2016