Skip to content

module: improve require() performance

Rodrigo Muino Tomonari requested to merge github/fork/mscdex/module-require-perf into master

Benchmark results for the changes in this PR:

                                                                        improvement significant      p.value
 module/module-loader.js useCache="false" fullPath="false" thousands=50      5.67 %         *** 8.756066e-18
 module/module-loader.js useCache="false" fullPath="true" thousands=50       6.33 %         *** 1.120897e-16
 module/module-loader.js useCache="true" fullPath="false" thousands=50     125.20 %         *** 4.165830e-37
 module/module-loader.js useCache="true" fullPath="true" thousands=50      131.80 %         *** 2.755969e-34

First off, I recognize the module module is "locked" so I understand there is a possibility only some or none of these commits may be accepted. Either way I have initially marked this as semver-major because of a couple of changes, if they end up not being an issue I will happily remove the semver-major label:

The first is mscdex/io.js@eb3b7178fe8021de7dcb8402d3988cc11ac2a04d which changes the format of the key used in Module._pathCache. Instead of using JSON, a simple delimiter is used between the string values being used. This commit probably provides the single largest performance increase by itself, however I am not sure just how many people are making assumptions about the format of the cache keys themselves. A quick search on github at least reveals that there are people directly accessing Module._pathCache, but they seem to just be iterating over the properties and deleting them when they include some substring (typically a module name the user is wanting to "uncache", in addition to deleting from require.cache).

Secondly, there are the changes in mscdex/io.js@3d8528d5060d9cc27d1b335925d51622c5e87ec6, which may cause issues if anyone is both directly accessing any of those objects and using obj.hasOwnProperty().

All of the other changes should be backwards compatible AFAIK.

CI: https://ci.nodejs.org/job/node-test-pull-request/5845/ CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/523/

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
  • benchmark
  • fs
  • module

Merge request reports