-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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 #57370
base: main
Are you sure you want to change the base?
assert: improve partialDeepStrictEqual #57370
Conversation
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.
The current settings deactivate the extraProps handling, due to the current implementation failing on these cases.
Review requested:
|
Benchmark https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1674/ Local results (only three stars)
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #57370 +/- ##
==========================================
+ Coverage 89.02% 90.22% +1.20%
==========================================
Files 665 630 -35
Lines 193408 185070 -8338
Branches 37275 36226 -1049
==========================================
- Hits 172180 166981 -5199
+ Misses 13891 11056 -2835
+ Partials 7337 7033 -304
🚀 New features to boost your workflow:
|
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.
This fixes holey array handling. These were a bit more tricky with the partial implementation. It also adds multiple tests and makes sure the implementation always checks for being a drop-in implementation for assert.deepStrictEqual()
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.
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.
@@ -245,6 +291,7 @@ function innerDeepEqual(val1, val2, strict, memos) { | |||
|
|||
const message1Enumerable = ObjectPrototypePropertyIsEnumerable(val1, 'message'); | |||
const name1Enumerable = ObjectPrototypePropertyIsEnumerable(val1, 'name'); | |||
// TODO(BridgeAR): Adjust cause and errors properties for partial mode. |
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.
Should this be resolved before we call the implementation RC?
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 would have removed the experimental warning with it applied :)
I can't think of more cases needing further handling.
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.
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 besides the non-enumerable
cause
anderrors
properties on errors. I guess that can land later. I would also
feel comfortable to change the stability to stable after this
lands.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.
This includes a performance improvement for
assert.deepStrictEqual()
.A few uncommon cases are actually slower without me being
able to detect the reason for this difference. It's related to
object comparison.
I noticed an interesting thing while working on this.
ObjectPrototypeHasOwnProperty
is significantly faster thanObjectHasOwn
. This is something we could take advantageof in other parts of the code and I plan on opening an issue for
V8 to fix that.