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

assert: improve partialDeepStrictEqual performance and add benchmark #56555

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

puskin94
Copy link
Contributor

now that #54630 has landed, I took the liberty to review the code I wrote to look for improvements that would squeeze some more performance out of the comparison mechanism

  • simplified some logic
  • reviewed loops and introduced more breaking points to make them run for less cycles
  • added a benchmark file to test future possible regressions when it comes to partialDeepStrictEqual

Refs: #54630

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. needs-ci PRs that need a full CI run. labels Jan 10, 2025
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.20%. Comparing base (6b3937a) to head (762399f).
Report is 30 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56555      +/-   ##
==========================================
+ Coverage   89.17%   89.20%   +0.02%     
==========================================
  Files         662      662              
  Lines      191672   191811     +139     
  Branches    36884    36930      +46     
==========================================
+ Hits       170922   171100     +178     
+ Misses      13614    13563      -51     
- Partials     7136     7148      +12     
Files with missing lines Coverage Δ
lib/assert.js 99.31% <100.00%> (+0.27%) ⬆️

... and 39 files with indirect coverage changes

lib/assert.js Outdated
@@ -514,17 +524,15 @@ function partiallyCompareArrays(actual, expected, comparedObjects) {
return false;
}

if (isDeepEqual === undefined) lazyLoadComparison();
if (typeof isDeepStrictEqual === 'undefined') lazyLoadComparison();
Copy link
Member

Choose a reason for hiding this comment

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

I would keep the former part. Using typeof should not change the speed.
We can also keep it consistent with isDeepEqual.

Suggested change
if (typeof isDeepStrictEqual === 'undefined') lazyLoadComparison();
if (isDeepEqual === undefined) lazyLoadComparison();

lib/assert.js Outdated
if (expectedItem === 0) {
const zeroKey = getZeroKey(expectedItem);
expectedCounts.set(zeroKey, (expectedCounts.get(zeroKey)?.count || 0) + 1);
expectedCounts.set(zeroKey, (expectedCounts.get(zeroKey) || 0) + 1);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expectedCounts.set(zeroKey, (expectedCounts.get(zeroKey) || 0) + 1);
expectedCounts.set(zeroKey, (expectedCounts.get(zeroKey) ?? 0) + 1);

lib/assert.js Outdated
@@ -554,6 +559,7 @@ function partiallyCompareArrays(actual, expected, comparedObjects) {
} else {
expectedCounts.set(zeroKey, count - 1);
}
continue;
Copy link
Member

Choose a reason for hiding this comment

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

The continue won't be needed, since the if statement will end and all other operations in the loop are also done. It won't impact the performance anymore and it is normally easier to read without continue statements.

Suggested change
continue;

lib/assert.js Outdated
Comment on lines 487 to 489
for (const actualItem of actualIterator) {
actualMap.set(actualItem, (actualMap.get(actualItem) || 0) + 1);
}
Copy link
Member

Choose a reason for hiding this comment

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

I believe the count of each item is always exactly 1. The reason is, that it's either a primitive or an object and an object reference is unique as well. Otherwise the entry would be overwritten while setting the value.

lib/assert.js Outdated

for (const expectedItem of expectedIterator) {
let foundMatch = false;
for (const { 0: actualItem, 1: count } of actualMapIterator) {
Copy link
Member

Choose a reason for hiding this comment

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

We only have to look for entries that are not primitives. That way we safe a lot of iterations. You can look at the code how it's down in the set comparison of isDeepStrictEqual(). It does exactly that. It can even be a bit simpler, because this does not require the non-strict checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BridgeAR very good point, I didn't think about it!

@puskin94 puskin94 force-pushed the partial-deep-strict-equal-perf branch from ac146c0 to 762399f Compare January 13, 2025 15:05
@puskin94 puskin94 requested a review from BridgeAR January 17, 2025 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants