module: add cache of request and filename to optimize require perf #21404

ghost · 2018-06-19T13:43:37Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

mscdex · 2018-06-19T16:26:55Z

lib/internal/modules/cjs/loader.js

  return filename;
 };
+Module._resolveFilename.cache = new Map();


Why are we attaching this to a function?

Thanks for your review. Here is the context: #21300
Make a internal cache for request and filename to avoid the additional performance cost

What I meant was why attach an object to a function value vs. say adding Module._filenameCache and using that instead? I also get that this is how the stat() function is currently working, but I am not sure how adding properties to function values affects performance of that function or the property being added as far as V8 is concerned.

At the very least, I think it would be a good idea to test both methods and see if there is any change in performance.

I am not sure how adding properties to function values affects performance of that function or the property being added as far as V8 is concerned

It should make no difference.

mscdex · 2018-06-19T16:28:05Z

lib/internal/modules/cjs/loader.js

  if (!filename) {
    // eslint-disable-next-line no-restricted-syntax
    var err = new Error(`Cannot find module '${request}'`);
    err.code = 'MODULE_NOT_FOUND';
    throw err;
  }
+  if (parentFilename) {
+    const tempObj = cache.get(parentFilename) || {};


Perhaps Object.create(null) should be used instead of {}.

Thanks, I'll modify this

mscdex · 2018-06-19T16:28:39Z

lib/internal/modules/cjs/loader.js

  return filename;
 };
+Module._resolveFilename.cache = new Map();


Also, is using Map instead of a plain object (e.g. Object.create(null)) actually faster?

if (depth === 0) stat.cache = new Map();

I saw that stat.cache inside this file used Map for the cache container, based on what is considered?

mscdex · 2018-06-19T16:29:28Z

lib/internal/modules/cjs/loader.js

+  // 3. don't have cache for this parent.filename
+  if (!(options && options.paths) &&
+      !parentFilename &&
+      cache.get(parentFilename) !== undefined) {


Doing the lookup twice is wasteful. I'd lookup and store the value the first time and then check if that stored value is undefined and use it again below.

Thanks, I'll modify this.

mapleeit · 2018-06-20T03:10:19Z

Hi @bnoordhuis , I re-think about making a global cache instead of a cache in the Module constructor. There may have some side effects when delete require.cache[filename]. The cache will not be deleted as expected.

starkwang · 2018-06-21T12:27:02Z

lib/internal/modules/cjs/loader.js

  if (!filename) {
    // eslint-disable-next-line no-restricted-syntax
    var err = new Error(`Cannot find module '${request}'`);
    err.code = 'MODULE_NOT_FOUND';
    throw err;
  }
+  if (parentFilename) {
+    const tempObj = cache.get(parentFilename) || {};


tempObj is not a good name for variable, maybe filenameCache ?

starkwang · 2018-06-21T12:37:54Z

lib/internal/modules/cjs/loader.js

+  // 2. don't have parent.filename
+  // 3. don't have cache for this parent.filename
+  if (!(options && options.paths) &&
+      !parentFilename &&


Umm... Should this line change into parentFilename && ... ? It should be truthy when using the cache.

starkwang · 2018-06-21T12:43:11Z

lib/internal/modules/cjs/loader.js

+  // do not use the cache
+  // 1. require.resolve('you/request', {paths: []})
+  // 2. don't have parent.filename
+  // 3. don't have cache for this parent.filename


Comments here is a little bit confusing. It's better to say "Using the cache when ... "

starkwang · 2018-06-21T12:51:23Z

It's better to add a benchmark for this change. You can refer to the guide for writing and running benchmarks.

mapleeit · 2018-06-21T13:09:35Z

@starkwang Thanks for your review and advice! I'll append benchmark later.

But here is still a problem to discuss. Using a global cache or using a cache in the parent module. @bnoordhuis advised that use a global cache like the existing stat.cache. But this will not be deleted when people use delete require.cache[filename]. I'm a little concerned about that a global cache may cause some side effects when deleting cache.

Do you have any advise for this?

ghost · 2018-07-11T09:28:46Z

But here is still a problem to discuss. Using a global cache or using a cache in the parent module. @bnoordhuis advised that use a global cache like the existing stat.cache. But this will not be deleted when people use delete require.cache[filename]. I'm a little concerned about that a global cache may cause some side effects when deleting cache.

ping @bnoordhuis

jasnell · 2018-09-10T18:21:38Z

What's the status on this one?

mapleeit · 2019-02-22T07:44:03Z

Added benchmark.

benchmark result:

jdalton · 2019-02-22T08:32:12Z

lib/internal/modules/cjs/loader.js

+      if (filename) return filename;
+    }
+  }
+


Instead of caching per parent would it be more beneficial to cache from the path.dirname(parent.filename). Something like:

cacheKey = request + "\0" + fromPath + "\0" + (isMain ? "1" : "")

Do you mean that using a global cache, which uses request + parentDirName + isMain as keys?
But this will not be deleted when people use delete require.cache[filename]. I'm a little concerned about that a global cache may cause some side effects when deleting cache.

fhinkel · 2019-10-26T23:15:25Z

Ping @mapleeit

mapleeit · 2019-10-31T11:56:51Z

@fhinkel yeah, I'm here. Sorry for the suspending. I'll review this one again this weekend.

guybedford · 2019-10-31T18:31:14Z

A lot of other caching improvements have landed since this PR was made (#26970 is exactly a parent resolution cache like this PR). Perhaps that removes the need for this PR now?

mapleeit · 2019-11-01T03:10:50Z

@guybedford Thanks for pointing it out. Glad to know there is already a merged PR. I'm closing this one.

ghost mentioned this pull request Jun 19, 2018

Require perf optimization #21300

Closed

4 tasks

mscdex reviewed Jun 19, 2018

View reviewed changes

starkwang reviewed Jun 21, 2018

View reviewed changes

jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018

jdalton reviewed Feb 22, 2019

View reviewed changes

module: add cache of request and filename to optimize require perf

e1a906c

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:52

ghost closed this Nov 1, 2019

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: add cache of request and filename to optimize require perf #21404

module: add cache of request and filename to optimize require perf #21404

ghost commented Jun 19, 2018

mscdex Jun 19, 2018

mapleeit Jun 19, 2018

mscdex Jun 19, 2018 •

edited

Loading

bnoordhuis Jun 19, 2018

mscdex Jun 19, 2018

mapleeit Jun 19, 2018

mscdex Jun 19, 2018

mapleeit Jun 19, 2018

mscdex Jun 19, 2018 •

edited

Loading

mapleeit Jun 19, 2018

mapleeit commented Jun 20, 2018

starkwang Jun 21, 2018

starkwang Jun 21, 2018

starkwang Jun 21, 2018

starkwang commented Jun 21, 2018

mapleeit commented Jun 21, 2018

ghost commented Jul 11, 2018

jasnell commented Sep 10, 2018

mapleeit commented Feb 22, 2019 •

edited

Loading

jdalton Feb 22, 2019 •

edited

Loading

mapleeit Feb 22, 2019

fhinkel commented Oct 26, 2019

mapleeit commented Oct 31, 2019

guybedford commented Oct 31, 2019

mapleeit commented Nov 1, 2019

module: add cache of request and filename to optimize require perf #21404

module: add cache of request and filename to optimize require perf #21404

Conversation

ghost commented Jun 19, 2018

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Jun 19, 2018 • edited Loading

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

Choose a reason for hiding this comment

mscdex Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleeit commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starkwang commented Jun 21, 2018

mapleeit commented Jun 21, 2018

ghost commented Jul 11, 2018

jasnell commented Sep 10, 2018

mapleeit commented Feb 22, 2019 • edited Loading

jdalton Feb 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhinkel commented Oct 26, 2019

mapleeit commented Oct 31, 2019

guybedford commented Oct 31, 2019

mapleeit commented Nov 1, 2019

mscdex Jun 19, 2018 •

edited

Loading

mscdex Jun 19, 2018 •

edited

Loading

mapleeit commented Feb 22, 2019 •

edited

Loading

jdalton Feb 22, 2019 •

edited

Loading