util,assert: improve comparison performance - #22197
Conversation
Sorry, something went wrong.
|
@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/464/pipeline |
Sorry, something went wrong.
|
Perhaps the V8 patch should be done in a separate PR for easier management? Additionally, the v8_embedder_string in common.gypi needs to be updated whenever a V8 patch is applied. |
Sorry, something went wrong.
benjamingr
left a comment
There was a problem hiding this comment.
There's a ton of stuff here:
- There's a refactoring of the buffer benchmarks
- There's a change to make comparisons.js safe to tampering of primitives
- There's a v8 patch and tests for it
- There's a complete rewrite of strictDeepEqual
I don't understand why it's all in one PR - it makes it hard to review.
In general terms I'm very +1 on the changes and it looks like good code - it's just hard to review right now.
Sorry, something went wrong.
|
@mscdex thanks, I forgot about that. It is now fixed. I have all the things together in one PR because they mainly rely on each other and quite a few would cause conflicts otherwise. Since you both would rather have the PR split up, I'll do that tough. |
Sorry, something went wrong.
|
I moved some parts and I'll wait until those land and then rebase this PR. |
Sorry, something went wrong.
|
CI https://ci.nodejs.org/job/node-test-pull-request/16467/ |
Sorry, something went wrong.
|
Here the benchmark results for strict mode: These performance improvements are on top of the ones in #22258. Benchmarkassert/deepequal-buffer.js method='deepEqual' strict=1 len=1000 n=20000 *** 1510.77 % ±62.77% ±83.85% ±109.81% assert/deepequal-buffer.js method='deepEqual' strict=1 len=100 n=20000 *** 186.07 % ±9.69% ±12.92% ±16.86% assert/deepequal-buffer.js method='notDeepEqual' strict=1 len=1000 n=20000 5.11 % ±6.11% ±8.10% ±10.47% assert/deepequal-buffer.js method='notDeepEqual' strict=1 len=100 n=20000 4.98 % ±7.53% ±9.98% ±12.91% assert/deepequal-map.js method='deepEqual_mixed' strict=1 len=500 n=500 *** 10.07 % ±2.03% ±2.69% ±3.47% assert/deepequal-map.js method='deepEqual_objectOnly' strict=1 len=500 n=500 *** 12.10 % ±3.54% ±4.72% ±6.16% assert/deepequal-map.js method='deepEqual_primitiveOnly' strict=1 len=500 n=500 -1.33 % ±4.69% ±6.21% ±8.03% assert/deepequal-map.js method='notDeepEqual_mixed' strict=1 len=500 n=500 -1.70 % ±5.68% ±7.53% ±9.75% assert/deepequal-map.js method='notDeepEqual_objectOnly' strict=1 len=500 n=500 *** 12.22 % ±2.23% ±2.96% ±3.83% assert/deepequal-map.js method='notDeepEqual_primitiveOnly' strict=1 len=500 n=500 0.31 % ±4.13% ±5.47% ±7.07% assert/deepequal-object.js method='deepEqual' strict=1 size=1000 n=5000 -2.98 % ±4.03% ±5.35% ±6.91% assert/deepequal-object.js method='deepEqual' strict=1 size=100 n=5000 -0.32 % ±5.36% ±7.11% ±9.19% assert/deepequal-object.js method='deepEqual' strict=1 size=50000 n=5000 *** 10.37 % ±0.84% ±1.11% ±1.43% assert/deepequal-object.js method='notDeepEqual' strict=1 size=1000 n=5000 *** 27.00 % ±7.68% ±10.18% ±13.16% assert/deepequal-object.js method='notDeepEqual' strict=1 size=100 n=5000 ** 10.18 % ±6.42% ±8.51% ±11.01% assert/deepequal-object.js method='notDeepEqual' strict=1 size=50000 n=5000 *** 574.60 % ±12.70% ±16.96% ±22.20% assert/deepequal-prims-and-objs-big-array-set.js method='deepEqual_Array' strict=1 len=20000 n=25 primitive='array' *** 217.97 % ±10.84% ±14.45% ±18.86% assert/deepequal-prims-and-objs-big-array-set.js method='deepEqual_Array' strict=1 len=20000 n=25 primitive='number' *** 225.33 % ±14.02% ±18.68% ±24.36% assert/deepequal-prims-and-objs-big-array-set.js method='deepEqual_Array' strict=1 len=20000 n=25 primitive='object' *** 210.40 % ±7.74% ±10.28% ±13.32% assert/deepequal-prims-and-objs-big-array-set.js method='deepEqual_Array' strict=1 len=20000 n=25 primitive='string' *** 210.77 % ±14.55% ±19.39% ±25.30% assert/deepequal-prims-and-objs-big-array-set.js method='deepEqual_Set' strict=1 len=20000 n=25 primitive='array' 3.58 % ±5.43% ±7.19% ±9.30% assert/deepequal-prims-and-objs-big-array-set.js method='deepEqual_Set' strict=1 len=20000 n=25 primitive='number' -1.19 % ±6.23% ±8.26% ±10.68% assert/deepequal-prims-and-objs-big-array-set.js method='deepEqual_Set' strict=1 len=20000 n=25 primitive='object' 0.43 % ±5.42% ±7.18% ±9.28% assert/deepequal-prims-and-objs-big-array-set.js method='deepEqual_Set' strict=1 len=20000 n=25 primitive='string' -0.61 % ±5.86% ±7.76% ±10.04% assert/deepequal-prims-and-objs-big-array-set.js method='notDeepEqual_Array' strict=1 len=20000 n=25 primitive='array' *** 226.74 % ±12.90% ±17.19% ±22.41% assert/deepequal-prims-and-objs-big-array-set.js method='notDeepEqual_Array' strict=1 len=20000 n=25 primitive='number' *** 248.77 % ±15.11% ±20.15% ±26.31% assert/deepequal-prims-and-objs-big-array-set.js method='notDeepEqual_Array' strict=1 len=20000 n=25 primitive='object' *** 225.71 % ±11.54% ±15.36% ±20.02% assert/deepequal-prims-and-objs-big-array-set.js method='notDeepEqual_Array' strict=1 len=20000 n=25 primitive='string' *** 219.05 % ±14.17% ±18.89% ±24.65% assert/deepequal-prims-and-objs-big-array-set.js method='notDeepEqual_Set' strict=1 len=20000 n=25 primitive='array' 4.85 % ±8.10% ±10.75% ±13.94% assert/deepequal-prims-and-objs-big-array-set.js method='notDeepEqual_Set' strict=1 len=20000 n=25 primitive='number' * 8.82 % ±6.70% ±8.88% ±11.48% assert/deepequal-prims-and-objs-big-array-set.js method='notDeepEqual_Set' strict=1 len=20000 n=25 primitive='object' 3.88 % ±6.65% ±8.82% ±11.41% assert/deepequal-prims-and-objs-big-array-set.js method='notDeepEqual_Set' strict=1 len=20000 n=25 primitive='string' -1.03 % ±5.57% ±7.40% ±9.62% assert/deepequal-prims-and-objs-big-loop.js method='deepEqual' strict=1 n=20000 primitive='array' * 5.79 % ±4.68% ±6.20% ±8.02% assert/deepequal-prims-and-objs-big-loop.js method='deepEqual' strict=1 n=20000 primitive='number' 3.38 % ±4.70% ±6.23% ±8.06% assert/deepequal-prims-and-objs-big-loop.js method='deepEqual' strict=1 n=20000 primitive='object' ** 7.03 % ±4.76% ±6.31% ±8.16% assert/deepequal-prims-and-objs-big-loop.js method='deepEqual' strict=1 n=20000 primitive='string' 2.62 % ±4.60% ±6.10% ±7.89% assert/deepequal-prims-and-objs-big-loop.js method='notDeepEqual' strict=1 n=20000 primitive='array' *** 10.33 % ±5.99% ±7.95% ±10.32% assert/deepequal-prims-and-objs-big-loop.js method='notDeepEqual' strict=1 n=20000 primitive='number' ** 8.16 % ±4.97% ±6.59% ±8.52% assert/deepequal-prims-and-objs-big-loop.js method='notDeepEqual' strict=1 n=20000 primitive='object' *** 8.89 % ±5.18% ±6.87% ±8.88% assert/deepequal-prims-and-objs-big-loop.js method='notDeepEqual' strict=1 n=20000 primitive='string' ** 9.07 % ±6.11% ±8.09% ±10.47% assert/deepequal-set.js method='deepEqual_mixed' strict=1 len=500 n=500 *** 13.06 % ±1.94% ±2.56% ±3.32% assert/deepequal-set.js method='deepEqual_objectOnly' strict=1 len=500 n=500 *** 11.46 % ±1.57% ±2.09% ±2.71% assert/deepequal-set.js method='deepEqual_primitiveOnly' strict=1 len=500 n=500 -3.53 % ±4.77% ±6.31% ±8.16% assert/deepequal-set.js method='notDeepEqual_mixed' strict=1 len=500 n=500 2.25 % ±4.51% ±6.00% ±7.79% assert/deepequal-set.js method='notDeepEqual_objectOnly' strict=1 len=500 n=500 *** 10.32 % ±2.12% ±2.81% ±3.64% assert/deepequal-set.js method='notDeepEqual_primitiveOnly' strict=1 len=500 n=500 -0.92 % ±3.52% ±4.67% ±6.03% assert/deepequal-typedarrays.js len=100 method='deepEqual' strict=1 n=500 type='Float32Array' *** 99.81 % ±11.33% ±15.09% ±19.66% assert/deepequal-typedarrays.js len=100 method='deepEqual' strict=1 n=500 type='Float64Array' *** 95.37 % ±7.22% ±9.59% ±12.44% assert/deepequal-typedarrays.js len=100 method='deepEqual' strict=1 n=500 type='Int8Array' *** 103.86 % ±11.88% ±15.75% ±20.39% assert/deepequal-typedarrays.js len=100 method='deepEqual' strict=1 n=500 type='Uint8Array' *** 99.22 % ±11.20% ±14.92% ±19.44% assert/deepequal-typedarrays.js len=100 method='deepEqual' strict=1 n=500 type='Uint8ClampedArray' *** 92.33 % ±10.14% ±13.47% ±17.50% assert/deepequal-typedarrays.js len=100 method='notDeepEqual' strict=1 n=500 type='Float32Array' * 6.62 % ±6.57% ±8.71% ±11.27% assert/deepequal-typedarrays.js len=100 method='notDeepEqual' strict=1 n=500 type='Float64Array' 5.79 % ±6.69% ±8.87% ±11.47% assert/deepequal-typedarrays.js len=100 method='notDeepEqual' strict=1 n=500 type='Int8Array' ** 13.83 % ±8.80% ±11.72% ±15.26% assert/deepequal-typedarrays.js len=100 method='notDeepEqual' strict=1 n=500 type='Uint8Array' 2.87 % ±5.09% ±6.75% ±8.73% assert/deepequal-typedarrays.js len=100 method='notDeepEqual' strict=1 n=500 type='Uint8ClampedArray' 0.70 % ±5.73% ±7.60% ±9.82% assert/deepequal-typedarrays.js len=5000 method='deepEqual' strict=1 n=500 type='Float32Array' *** 2335.33 % ±79.73% ±106.50% ±139.47% assert/deepequal-typedarrays.js len=5000 method='deepEqual' strict=1 n=500 type='Float64Array' *** 2033.88 % ±115.98% ±154.94% ±202.90% assert/deepequal-typedarrays.js len=5000 method='deepEqual' strict=1 n=500 type='Int8Array' *** 2869.75 % ±144.70% ±193.30% ±253.14% assert/deepequal-typedarrays.js len=5000 method='deepEqual' strict=1 n=500 type='Uint8Array' *** 2840.20 % ±147.94% ±197.63% ±258.81% assert/deepequal-typedarrays.js len=5000 method='deepEqual' strict=1 n=500 type='Uint8ClampedArray' *** 2824.27 % ±131.85% ±176.13% ±230.65% assert/deepequal-typedarrays.js len=5000 method='notDeepEqual' strict=1 n=500 type='Float32Array' * 8.54 % ±6.78% ±9.01% ±11.72% assert/deepequal-typedarrays.js len=5000 method='notDeepEqual' strict=1 n=500 type='Float64Array' ** 9.54 % ±6.63% ±8.81% ±11.45% assert/deepequal-typedarrays.js len=5000 method='notDeepEqual' strict=1 n=500 type='Int8Array' 2.08 % ±5.25% ±6.96% ±9.00% assert/deepequal-typedarrays.js len=5000 method='notDeepEqual' strict=1 n=500 type='Uint8Array' 1.98 % ±6.92% ±9.17% ±11.85% assert/deepequal-typedarrays.js len=5000 method='notDeepEqual' strict=1 n=500 type='Uint8ClampedArray' * 9.76 % ±9.12% ±12.13% ±15.78% |
Sorry, something went wrong.
|
PTAL @nodejs/util @nodejs/testing |
Sorry, something went wrong.
|
@joyeecheung @addaleax [EDIT: Used git shortlog to get an idea of who might be most familiar with the files modified here. I can give this a review, but I'd want someone else to review the C++ part especially.] |
Sorry, something went wrong.
Sorry, something went wrong.
|
Rerun the CI https://ci.nodejs.org/job/node-test-commit/20612/ ✔️ |
Sorry, something went wrong.
|
It would be great to get a review. I have a follow up optimization for util inspect that requires the same c++ function and I wait for opening the PR until this lands. |
Sorry, something went wrong.
|
I'm sorry, I read the code and it looks good to me but I don't understand it well enough to LGTM and would prefer it if someone who has more experience with the API review it. If you don't get a review in 48 hours ping me and I'll go through the V8 changes very carefully and review. Also, pinging benedikt or maya for when they're back might be a good idea @bmeurer @MayaLekova |
Sorry, something went wrong.
ryzokuken
left a comment
There was a problem hiding this comment.
LGTM, just a few minor doubts.
Sorry, something went wrong.
There was a problem hiding this comment.
I couldn't see if we have a check for this in-place. Will we crash if this fails? That said, does it ever really realistically fail?
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, it would crash. But this API is only used internally and not exposed. That's why I did not add a safeguard against wrong input values.
Sorry, something went wrong.
There was a problem hiding this comment.
I added a check for that as well.
Sorry, something went wrong.
There was a problem hiding this comment.
Same as above, does this ever fail? Instead, you could try something like
if(!object->GetPropertyNames(...).ToLocal(&properties)) return;
Sorry, something went wrong.
There was a problem hiding this comment.
I am not aware about this. I use it as it's done in the V8 tests. @nodejs/v8 do we have to add a guard against this?
Sorry, something went wrong.
There was a problem hiding this comment.
I think you do, yes. The whole point of ToLocalChecked is that you must've checked the value earlier. You could either add a CHECK statement before this one, in which case it'll still crash (probably in a much better manner, but crash nonetheless) or use ToLocal as I did above.
Sorry, something went wrong.
There was a problem hiding this comment.
Since the original behavior of Object.keys is to throw if the keys cannot be collected, it makes more sense to return if the maybe is empty and throw in the JS land. (I guess this should fail if you throw an error in the enumerate proxy handler, though it is deprecated? EDIT: nope, it's not in V8 anymore)
Sorry, something went wrong.
There was a problem hiding this comment.
Exactly. Returning (throwing) if the MaybeLocal is empty seems to be the most logical thing to do. In V8, one would use the macro ASSIGN_RETURN_FAILURE_ON_EXCEPTION which is essentially:
if (!call(...)->ToLocal(&result) {
DCHECK(isolate->has_pending_exception());
return;
}
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, please use @ryzokuken’s suggestion here, I think this could otherwise fail when inspecting Proxies with an ownKeys handler. A CHECK() would have the same behaviour as ToLocalChecked(), so that’s not enough either.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks a lot! I learned something through your comments 👍. It is now handled as suggested.
Sorry, something went wrong.
addaleax
left a comment
There was a problem hiding this comment.
LGTM once @ryzokuken’s comment is addressed
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, please use @ryzokuken’s suggestion here, I think this could otherwise fail when inspecting Proxies with an ownKeys handler. A CHECK() would have the same behaviour as ToLocalChecked(), so that’s not enough either.
Sorry, something went wrong.
Sorry, something went wrong.
|
@BridgeAR This needs to be rebased against master due to the recent CI hiccup. (Also, it looks like @addaleax signed off on this but did not use the green checkmark, so maybe add their Reviewed-By: manually if ncu doesn't pick it up and if you're feeling up to it!) |
Sorry, something went wrong.
This significantly improves regular and typed array performance by not checking the indices keys anymore. This can be done with a V8 API that allows to only retrieve the non indices property keys.
|
Rebased. I squashed the reviewed part and kept the other part. |
Sorry, something went wrong.
addaleax
left a comment
There was a problem hiding this comment.
LGTM, thanks!
Sorry, something went wrong.
This significantly improves regular and typed array performance by not checking the indices keys anymore. This can be done with a V8 API that allows to only retrieve the non indices property keys. PR-URL: nodejs#22197 Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
|
Thanks for the reviews! Landed in 3479b1c 🎉 |
Sorry, something went wrong.
bmeurer
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
This significantly improves regular and typed array performance by not checking the indices keys anymore. This can be done with a V8 API that allows to only retrieve the non indices property keys. PR-URL: #22197 Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
This significantly improves regular and typed array performance by not checking the indices keys anymore. This can be done with a V8 API that allows to only retrieve the non indices property keys. PR-URL: #22197 Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
This significantly improves the performance of util.isDeepStrictEqual and assert.deepStrictEqual.
Update:
I removed a lot of the original optimization. This is now condensed to the new API usage that allows to skip indices. This API is useful for all (typed) arrays. Depending on the input it is a huge performance boost.
These performance improvements are on top of the ones in #22258.
BenchmarkChecklist