Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[utils] [perf] Performance of fullResolve #2755

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

leipert
Copy link
Contributor

@leipert leipert commented Apr 8, 2023

While looking at a larger code base https://gitlab.com/gitlab-org/gitlab, I've came to realize that fullResolve takes a lot of CPU cycles, particularly the hashObject calls inside it.

I applied the following patch locally to see how often this is called and how many unique hashes were produced:

diff --git a/utils/resolve.js b/utils/resolve.js
index 4a35c6a..3c28324 100644
--- a/utils/resolve.js
+++ b/utils/resolve.js
@@ -83,13 +83,28 @@ function relative(modulePath, sourceFile, settings) {
   return fullResolve(modulePath, sourceFile, settings).path;
 }

+let prevSettings = null;
+let nonEqualSettings = 0;
+let totalCalls = 0;
+let uniqueHashes = new Set();
 function fullResolve(modulePath, sourceFile, settings) {
   // check if this is a bonus core module
   const coreSet = new Set(settings['import/core-modules']);
   if (coreSet.has(modulePath)) return { found: true, path: null };

   const sourceDir = path.dirname(sourceFile);
-  const cacheKey = sourceDir + hashObject(settings).digest('hex') + modulePath;
+
+  totalCalls+=1;
+  const hash = hashObject(settings).digest('hex');
+
+  if(prevSettings !== settings){
+    uniqueHashes.add(hash);
+    prevSettings = settings;
+    nonEqualSettings+=1;
+    console.log(`fullResolve | Total calls:${totalCalls} | Non-Equal settings:${nonEqualSettings} | Unique hashes:${uniqueHashes.size} | dir:${sourceDir}`)
+  }
+
+  const cacheKey = sourceDir + hash + modulePath;

   const cacheSettings = ModuleCache.getSettings(settings);

For our code base, fullResolve is called more than 570 thousand times. The simple in-equality !== code path is taken 1090 times. Actually only four unique hashes are produced, meaning we only have four unique settings across our code base.

I assume that a full object equality comparison might not be needed, and a simple object comparison with !== already would reduce the amount of hashObject calls by 570x. This is what is implemented in this commit.

Time spend in fullResolve was reduced by ~38%:

  • Before: 17% (19.10s) of our total execution time
  • After: 11% (11.86s) of our total execution time

The effect might even be more pronounced on machines that are slower when calculating sha256 hashes or that have less memory, as the hashObject method tends to create loads of small strings which need to be garbage collected.

While looking at a larger code base https://gitlab.com/gitlab-org/gitlab,
I've came to realize that `fullResolve` takes a lot of CPU cycles,
particularly the `hashObject` calls inside it.

I applied the following patch locally to see how often this is called and
how many unique hashes were produced:

```diff
diff --git a/utils/resolve.js b/utils/resolve.js
index 4a35c6a..3c28324 100644
--- a/utils/resolve.js
+++ b/utils/resolve.js
@@ -83,13 +83,28 @@ function relative(modulePath, sourceFile, settings) {
   return fullResolve(modulePath, sourceFile, settings).path;
 }

+let prevSettings = null;
+let nonEqualSettings = 0;
+let totalCalls = 0;
+let uniqueHashes = new Set();
 function fullResolve(modulePath, sourceFile, settings) {
   // check if this is a bonus core module
   const coreSet = new Set(settings['import/core-modules']);
   if (coreSet.has(modulePath)) return { found: true, path: null };

   const sourceDir = path.dirname(sourceFile);
-  const cacheKey = sourceDir + hashObject(settings).digest('hex') + modulePath;
+
+  totalCalls+=1;
+  const hash = hashObject(settings).digest('hex');
+
+  if(prevSettings !== settings){
+    uniqueHashes.add(hash);
+    prevSettings = settings;
+    nonEqualSettings+=1;
+    console.log(`fullResolve | Total calls:${totalCalls} | Non-Equal settings:${nonEqualSettings} | Unique hashes:${uniqueHashes.size} | dir:${sourceDir}`)
+  }
+
+  const cacheKey = sourceDir + hash + modulePath;

   const cacheSettings = ModuleCache.getSettings(settings);
```

For our code base, `fullResolve` is called more than 570 thousand times.
The simple in-equality `!==` code path is taken 1090 times.
Actually only _four_ unique hashes are produced, meaning we only have
four unique settings across our code base.

I assume that a full object equality comparison might not be needed,
and a simple object comparison with `!==` already would reduce the amount
of `hashObject` calls by 570x. This is what is implemented in this commit.

Time spend in `fullResolve` was reduced by ~38%:
- Before: 17% (19.10s) of our total execution time
- After: 11% (11.86s) of our total execution time

The effect might even be more pronounced on machines that are slower when
calculating `sha256` hashes or that have less memory, as the `hashObject`
method tends to create loads of small strings which need to be garbage
collected.
@leipert leipert force-pushed the leipert-utils-fullResolve-perf branch 3 times, most recently from 3adb842 to 712ab68 Compare April 8, 2023 14:16
@leipert leipert force-pushed the leipert-utils-fullResolve-perf branch from 712ab68 to 12d0bc3 Compare April 8, 2023 21:51
@leipert
Copy link
Contributor Author

leipert commented Apr 8, 2023

Rebased and ready for review 👍 Thank you!

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems simple, thanks!

Anyone mutating the settings object mid-run deserves to have things break :-)

@ljharb ljharb force-pushed the leipert-utils-fullResolve-perf branch from 12d0bc3 to 6cb9616 Compare April 11, 2023 19:13
@ljharb ljharb merged commit 6cb9616 into import-js:main Apr 11, 2023
@leipert leipert deleted the leipert-utils-fullResolve-perf branch April 12, 2023 12:03
@leipert
Copy link
Contributor Author

leipert commented Apr 12, 2023

Awesome, thanks @ljharb!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants