Skip to content

assert: improve partialDeepStrictEqual - #57370

Closed
BridgeAR wants to merge 7 commits into
nodejs:mainfrom
BridgeAR:fix-assert-partial-deep-strict-equal
Closed

assert: improve partialDeepStrictEqual#57370
BridgeAR wants to merge 7 commits into
nodejs:mainfrom
BridgeAR:fix-assert-partial-deep-strict-equal

Conversation

@BridgeAR

@BridgeAR BridgeAR commented Mar 8, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

This significantly improves the assert.partialDeepStrictEqual
implementation (see benchmark below) by reusing the already existing algorithm.
It is significantly faster and handles all known edge cases like symbols
identical as the deepStrictEqual algorithm. This is crucial to
remove the experimental status from the implementation.

The old implementation could not yet handle some cases that
the benchmark handles. These are deactivated for now.
This is likely also a good idea due to the significant performance
difference between the two implementations.

I changed the stability index, since it's now handling all cases
properly.

This includes a performance improvement for assert.deepStrictEqual().
A few uncommon cases are actually slower due to seemingly V8
deoptimising the code when hitting rare cases. In reality, that should
not be an issue.

I noticed an interesting thing while working on this.
ObjectPrototypeHasOwnProperty is significantly faster than
ObjectHasOwn. This is something we could take advantage
of in other parts of the code and I plan on opening an issue for
V8 to fix that.

Closes #56555

@nodejs-github-bot

nodejs-github-bot commented Mar 8, 2025

Copy link
Copy Markdown
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. util Issues and PRs related to the built-in util module. labels Mar 8, 2025
@BridgeAR BridgeAR added request-ci Add this label to start a Jenkins CI on a PR. needs-benchmark-ci PR that need a benchmark CI run. labels Mar 8, 2025
@BridgeAR

BridgeAR commented Mar 8, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

The new benchmark file somehow can't be applied.

Local results (only three stars. I believe I have never had a 500x improvement before :D )

                                                                                                                      confidence improvement accuracy (*)      (**)     (***)
assert/deepequal-map.js method='deepEqual_mixed' strict=0 len=500 n=2000                                                     ***      6.19 %       ±0.76%    ±1.02%    ±1.32%
assert/deepequal-map.js method='deepEqual_mixed' strict=1 len=500 n=2000                                                     ***      5.74 %       ±1.06%    ±1.41%    ±1.84%
assert/deepequal-map.js method='deepEqual_objectOnly' strict=0 len=500 n=2000                                                ***      7.72 %       ±0.92%    ±1.23%    ±1.60%
assert/deepequal-map.js method='deepEqual_objectOnly' strict=1 len=500 n=2000                                                ***      6.71 %       ±1.39%    ±1.87%    ±2.46%
assert/deepequal-map.js method='notDeepEqual_objectOnly' strict=0 len=500 n=2000                                             ***    -21.18 %       ±0.76%    ±1.02%    ±1.33%
assert/deepequal-map.js method='notDeepEqual_objectOnly' strict=1 len=500 n=2000                                             ***    -18.31 %       ±0.69%    ±0.92%    ±1.20%
assert/deepequal-object.js method='deepEqual' size=10000 n=50                                                                ***      3.77 %       ±0.49%    ±0.65%    ±0.84%
assert/deepequal-object.js method='deepStrictEqual' size=10000 n=50                                                          ***      3.25 %       ±0.61%    ±0.81%    ±1.05%
assert/deepequal-object.js method='notDeepStrictEqual' size=10000 n=50                                                       ***     -6.99 %       ±2.65%    ±3.53%    ±4.60%
assert/deepequal-prims-and-objs-big-loop.js method='deepEqual' strict=0 n=100000 primitive='array'                           ***     18.40 %       ±1.00%    ±1.33%    ±1.74%
assert/deepequal-prims-and-objs-big-loop.js method='deepEqual' strict=0 n=100000 primitive='set_object'                      ***      6.31 %       ±1.07%    ±1.42%    ±1.86%
assert/deepequal-prims-and-objs-big-loop.js method='deepEqual' strict=0 n=100000 primitive='set_simple'                      ***      8.53 %       ±2.25%    ±2.99%    ±3.89%
assert/deepequal-prims-and-objs-big-loop.js method='deepEqual' strict=1 n=100000 primitive='array'                           ***     18.13 %       ±0.65%    ±0.86%    ±1.12%
assert/deepequal-prims-and-objs-big-loop.js method='deepEqual' strict=1 n=100000 primitive='set_object'                      ***      6.01 %       ±1.02%    ±1.36%    ±1.77%
assert/deepequal-prims-and-objs-big-loop.js method='deepEqual' strict=1 n=100000 primitive='set_simple'                      ***      4.39 %       ±1.24%    ±1.65%    ±2.15%
assert/deepequal-prims-and-objs-big-loop.js method='notDeepEqual' strict=1 n=100000 primitive='array'                        ***     13.84 %       ±1.08%    ±1.44%    ±1.90%
assert/deepequal-prims-and-objs-big-loop.js method='notDeepEqual' strict=1 n=100000 primitive='circular'                     ***      7.07 %       ±1.25%    ±1.67%    ±2.18%
assert/deepequal-prims-and-objs-big-loop.js method='notDeepEqual' strict=1 n=100000 primitive='set_object'                   ***      5.56 %       ±1.02%    ±1.35%    ±1.76%
assert/deepequal-prims-and-objs-big-loop.js method='notDeepEqual' strict=1 n=100000 primitive='set_simple'                   ***      4.62 %       ±0.98%    ±1.31%    ±1.72%
assert/deepequal-set.js method='deepEqual_mixed' strict=0 len=500 n=1000                                                     ***      7.46 %       ±0.87%    ±1.15%    ±1.50%
assert/deepequal-set.js method='deepEqual_mixed' strict=1 len=500 n=1000                                                     ***      7.20 %       ±0.92%    ±1.22%    ±1.60%
assert/deepequal-set.js method='deepEqual_objectOnly' strict=0 len=500 n=1000                                                ***      7.29 %       ±0.58%    ±0.78%    ±1.01%
assert/deepequal-set.js method='deepEqual_objectOnly' strict=1 len=500 n=1000                                                ***     15.42 %       ±0.66%    ±0.88%    ±1.15%
assert/deepequal-set.js method='notDeepEqual_objectOnly' strict=0 len=500 n=1000                                             ***    -19.87 %       ±0.45%    ±0.60%    ±0.78%
assert/deepequal-set.js method='notDeepEqual_objectOnly' strict=1 len=500 n=1000                                             ***    -13.31 %       ±0.80%    ±1.07%    ±1.39%
assert/deepequal-simple-array-and-set.js method='deepEqual_Array' strict=1 len=10000 n=1000                                  ***    165.44 %       ±1.82%    ±2.43%    ±3.17%
assert/deepequal-simple-array-and-set.js method='notDeepEqual_Array' strict=1 len=10000 n=1000                               ***    166.21 %       ±1.69%    ±2.26%    ±2.94%
assert/deepequal-simple-array-and-set.js method='notDeepEqual_Set' strict=1 len=10000 n=1000                                 ***     44.09 %       ±3.55%    ±4.76%    ±6.26%
assert/deepequal-typedarrays.js len=100 method='deepEqual' strict=0 n=25000 type='Float32Array'                              ***      3.60 %       ±1.87%    ±2.49%    ±3.24%
assert/partial-deep-equal.js datasetName='array' extraProps=0 size=500 n=25                                                  ***  51153.52 %     ±794.65% ±1070.97% ±1421.83%
assert/partial-deep-equal.js datasetName='arrayBuffers' extraProps=0 size=500 n=25                                           ***    136.55 %       ±4.31%    ±5.80%    ±7.66%
assert/partial-deep-equal.js datasetName='circularRefs' extraProps=0 size=500 n=25                                           ***   2990.07 %      ±32.88%   ±44.31%   ±58.82%
assert/partial-deep-equal.js datasetName='dataViewArrayBuffers' extraProps=0 size=500 n=25                                   ***    139.93 %       ±4.34%    ±5.82%    ±7.67%
assert/partial-deep-equal.js datasetName='maps' extraProps=0 size=500 n=25                                                   ***   2114.76 %       ±5.24%    ±7.06%    ±9.37%
assert/partial-deep-equal.js datasetName='objects' extraProps=0 size=500 n=25                                                ***   7168.80 %      ±56.37%   ±75.98%  ±100.87%
assert/partial-deep-equal.js datasetName='sets' extraProps=0 size=500 n=25                                                   ***   1444.39 %       ±8.84%   ±11.91%   ±15.81%
assert/partial-deep-equal.js datasetName='typedArrays' extraProps=0 size=500 n=25                                            ***  14281.96 %      ±97.65%  ±131.61%  ±174.73%

@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2025
@nodejs-github-bot

nodejs-github-bot commented Mar 8, 2025

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65643/

@codecov

codecov Bot commented Mar 8, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.22%. Comparing base (a446e3b) to head (2ab10d6).
Report is 102 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #57370      +/-   ##
==========================================
- Coverage   90.23%   90.22%   -0.01%     
==========================================
  Files         630      629       -1     
  Lines      185211   184976     -235     
  Branches    36245    36206      -39     
==========================================
- Hits       167128   166898     -230     
+ Misses      11056    11037      -19     
- Partials     7027     7041      +14
Files with missing lines Coverage Δ
lib/assert.js 99.52% <100.00%> (+0.47%) ⬆️
lib/internal/util/comparisons.js 100.00% <100.00%> (ø)

... and 36 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@BridgeAR
BridgeAR requested a review from RafaelGSS March 8, 2025 17:19
aduh95
aduh95 reviewed Mar 8, 2025
View reviewed changes
Comment thread lib/internal/util/comparisons.js Outdated
@BridgeAR
BridgeAR force-pushed the fix-assert-partial-deep-strict-equal branch from bc255c4 to 0a2a533 Compare March 11, 2025 01:09
aduh95
aduh95 reviewed Mar 11, 2025
View reviewed changes
Comment thread doc/api/assert.md Outdated
@BridgeAR BridgeAR added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 12, 2025
@BridgeAR

BridgeAR commented Mar 12, 2025

Copy link
Copy Markdown
Member Author

I marked the PR as semver minor due to the removal of the experimental warning.

@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 12, 2025
@nodejs-github-bot

nodejs-github-bot commented Mar 12, 2025

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65694/

@BridgeAR
BridgeAR force-pushed the fix-assert-partial-deep-strict-equal branch from 0a2a533 to e053279 Compare March 12, 2025 12:09
@BridgeAR

BridgeAR commented Mar 12, 2025

Copy link
Copy Markdown
Member Author

PTAL.

I pushed another change that adds the changed part to the docs, marks a line as unreachable, and updated the examples in the documentation to be on multiple lines to easier read them.
This should be ready to land with a new approval.

BridgeAR added 6 commits March 12, 2025 13:15
@BridgeAR
assert: improve partialDeepStrictEqual
b41ac78 
This significantly improves the assert.partialDeepStrictEqual
implementation by reusing the already existing algorithm.
It is significantly faster and handles edge cases like symbols
identical as the deepStrictEqual algorithm. This is crucial to
remove the experimental status from the implementation.
@BridgeAR
benchmark: add assert partialDeepStrictEqual benchmark
15a81e6 
The current settings deactivate the extraProps handling, due to
the current implementation failing on these cases.
@BridgeAR
benchmark: skip running some assert benchmarks by default
5d7a3d3 
These benchmarks are not frequently needed and just slow down the
default benchmark suite. They are kept for users who want to run
them but deactivated by default.
@BridgeAR
benchmark: adjust assert runtimes
3a65b11 
Each file should have a reasonable runtime while having a good
accuracy. This adjust those up and down to have minimal runtimes
with a good accuracy.
@BridgeAR
assert,util: improve performance
5c40578 
This improves the performance for array comparison by making the
sparse array detection simpler. On top of that it adds a fast path
for sets and maps that only contain objects as key.
@BridgeAR
assert: implement partial error comparison
292d886 
assert.partialDeepStrictEqual now also handled error properties as
expected. On top of that, the main implementation also handles
non-string `name` and `message` properties and the comparison is a
tad faster by removing duplicated comparison steps.

As a drive-by fix this also cleans up some code by abstracting code
and renaming variables for clarity.
@RafaelGSS RafaelGSS mentioned this pull request Apr 8, 2025
Closed
@RafaelGSS RafaelGSS mentioned this pull request Apr 11, 2025
Merged
@RafaelGSS RafaelGSS mentioned this pull request May 1, 2025
Merged
@github-actions github-actions Bot mentioned this pull request Jun 10, 2025
Merged
@AlCalzone AlCalzone mentioned this pull request May 13, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 left review comments
@H4ad H4ad H4ad approved these changes
@RafaelGSS RafaelGSS RafaelGSS approved these changes

Assignees

No one assigned

Labels

assert Issues and PRs related to the assert subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. util Issues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@BridgeAR @nodejs-github-bot @H4ad @aduh95 @RafaelGSS