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

some spacing between functions would be nice

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

lib/fs.js

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

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 dc56390Compare 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 f6067f3Compare April 7, 2016 15:16

mscdex force-pushed the perf-module-loading branch from f6067f3 to ae9783dCompare 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 ae18bbeCompare April 14, 2016 18:59

mscdex merged commit ae18bbe into nodejs:masterApr 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

Uh oh!

lib: improve module loading performance#5172

lib: improve module loading performance #5172

Uh oh!

Conversation

mscdex commented Feb 10, 2016

Uh oh!

mscdex commented Feb 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvagg commented Feb 10, 2016

Uh oh!

mscdex commented Feb 10, 2016

Uh oh!

jasnell commented Feb 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Feb 19, 2016

Uh oh!

jasnell commented Feb 19, 2016

Uh oh!

ronkorving commented Feb 27, 2016

Uh oh!

mscdex commented Feb 27, 2016

Uh oh!

jasnell commented Mar 21, 2016

Uh oh!

mscdex commented Mar 21, 2016

Uh oh!

jasnell commented Mar 21, 2016

Uh oh!

mscdex commented Apr 7, 2016

Uh oh!

jasnell commented Apr 7, 2016

Uh oh!

mscdex commented Apr 14, 2016

Uh oh!

MylesBorins commented Apr 14, 2016

Uh oh!

mscdex commented Apr 14, 2016

Uh oh!

MylesBorins commented Apr 15, 2016

Uh oh!

mhdawson commented Apr 15, 2016

Uh oh!

MylesBorins commented Apr 19, 2016

Uh oh!

mscdex commented Apr 19, 2016

Uh oh!

MylesBorins commented Apr 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants