Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork34.1k
assert,util: improve deep comparison performance#61076
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
assert,util: improve deep comparison performance#61076
Conversation
| wellKnownConstructors.add(Float16Array); | ||
| } | ||
| .add(WeakSet) | ||
| .add(Float16Array); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's still behind a flag:
node/deps/v8/src/flags/flag-definitions.h
Lines 331 to 332 in05f8772
| V(js_float16array, \ | |
| "Float16Array, Math.f16round, DataView.getFloat16, DataView.setFloat16") \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That seems to just be a legacy part that was not cleaned up in V8. It is fully shipped, V8/Chromium has it enabled as well as all other engines and there is absolutely no way this will be removed again.
I think we should consider it shipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should remove the flag then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@aduh95 thank you!
Uh oh!
There was an error while loading.Please reload this page.
The property comparison of invalid dates regressed when startingto handle invalid dates as being equal.
This is a very small improvement for error creation.
4f7303b to641e674Comparecodecovbot commentedDec 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #61076 +/- ##======================================= Coverage 88.52% 88.53% ======================================= Files 703 703 Lines 208538 208529 -9 Branches 40214 40207 -7 =======================================+ Hits 184615 184621 +6- Misses 15921 15925 +4+ Partials 8002 7983 -19
🚀 New features to boost your workflow:
|
Uh oh!
There was an error while loading.Please reload this page.
| returnfalse; | ||
| for(constkeyofgetOwnSymbols(val1)){ | ||
| if(hasEnumerable(val1,key)){ | ||
| ArrayPrototypePush(keys1,key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
just a sidebar, is this actually faster thankeys1[keys1.length] = key;? (i may have asked this before)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I believe it is the same, while that method is also used before and it is more about the overall logic that should have an impact (less checks for being enumerable needed and no filtering).
| getEnumerables(val2,symbolKeysB).length!==0){ | ||
| returnfalse; | ||
| } | ||
| for(constkeyofgetOwnSymbols(val2)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this should probably be a normal for loop (or use SafeArrayIterator)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is the same as before and I don't think we should write more non-idiomatic code than absolutely required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
a normal for loop is quite idiomatic, but sure, if the perf here isn't important then that's fine
This combines minor performance improvements for edge cases as well as adding more tests for partialDeepStrictEqual, resolving a TODO, and fixing a bug while comparing invalid dates (not taking into account that extra properties might exist).
The performance gain is mostly for objects containing symbols. We do not have benchmarks for these and our current benchmarks will not show much difference.
See commit messages for details.