util,assert: improve comparison performance #22197

BridgeAR · 2018-08-08T16:33:07Z

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.

Benchmark

 assert/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%

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-08-08T16:33:08Z

@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

mscdex · 2018-08-08T17:29:06Z

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.

benjamingr

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.

BridgeAR · 2018-08-09T00:55:18Z

@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.

BridgeAR · 2018-08-09T12:37:28Z

I moved some parts and I'll wait until those land and then rebase this PR.

BridgeAR · 2018-08-15T17:21:12Z

CI https://ci.nodejs.org/job/node-test-pull-request/16467/
Benchmark https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/225/

BridgeAR · 2018-08-15T18:47:31Z

Here the benchmark results for strict mode:

These performance improvements are on top of the ones in #22258.

Benchmark

 assert/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%

BridgeAR · 2018-08-15T20:52:11Z

PTAL @nodejs/util @nodejs/testing

Trott · 2018-08-15T20:54:38Z

@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.]

BridgeAR · 2018-08-15T21:00:40Z

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

BridgeAR · 2018-08-17T08:31:58Z

Rerun the CI https://ci.nodejs.org/job/node-test-commit/20612/ ✔️

BridgeAR · 2018-08-17T08:46:58Z

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.

benjamingr · 2018-08-17T14:03:25Z

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

ryzokuken

LGTM, just a few minor doubts.

ryzokuken · 2018-08-17T15:12:59Z

src/node_util.cc

+  Environment* env = Environment::GetCurrent(args);
+  Local<Context> context = env->context();
+
+  v8::Local<v8::Object> object = args[0].As<v8::Object>();


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?

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.

I added a check for that as well.

ryzokuken · 2018-08-17T15:14:50Z

src/node_util.cc

+    ->GetPropertyNames(context, v8::KeyCollectionMode::kOwnOnly,
+                       v8::ONLY_ENUMERABLE,
+                       v8::IndexFilter::kSkipIndices)
+      .ToLocalChecked();


Same as above, does this ever fail? Instead, you could try something like

if(!object->GetPropertyNames(...).ToLocal(&properties)) return;

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?

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.

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)

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; }

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.

Thanks a lot! I learned something through your comments 👍. It is now handled as suggested.

addaleax

LGTM once @ryzokuken’s comment is addressed

addaleax · 2018-08-19T10:15:30Z

src/node_util.cc

+    ->GetPropertyNames(context, v8::KeyCollectionMode::kOwnOnly,
+                       v8::ONLY_ENUMERABLE,
+                       v8::IndexFilter::kSkipIndices)
+      .ToLocalChecked();


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.

BridgeAR · 2018-08-19T13:16:50Z

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

Trott · 2018-08-19T16:36:28Z

@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!)

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.

BridgeAR · 2018-08-19T18:32:09Z

Rebased. I squashed the reviewed part and kept the other part.

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

addaleax

LGTM, thanks!

addaleax · 2018-08-19T23:27:43Z

lib/internal/util/comparisons.js

@@ -4,6 +4,7 @@ const { compare } = process.binding('buffer');
 const { isArrayBufferView } = require('internal/util/types');
 const { internalBinding } = require('internal/bootstrap/loaders');
 const { isDate, isMap, isRegExp, isSet } = internalBinding('types');
+const { getOwnNonIndicesProperties } = process.binding('util');


I think this nit got lost because I had bad wifi :/ Sorry, should have been part of my previous review, but is totally non-blocking: I think this should be called getOwnNonIndexProperties, since the expression for the returned things in English would be “own non-index properties”

I addressed it while landing.

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>

BridgeAR · 2018-08-20T00:13:51Z

Thanks for the reviews!

Landed in 3479b1c 🎉

bmeurer

LGTM

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>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 8, 2018

BridgeAR mentioned this pull request Aug 8, 2018

util, assert: improve array comparison performance #22111

Closed

4 tasks

benjamingr reviewed Aug 8, 2018

View reviewed changes

BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Aug 9, 2018

BridgeAR mentioned this pull request Aug 11, 2018

util,assert: improve comparison performance #22258

Closed

4 tasks

BridgeAR force-pushed the improve-comparison-further branch from dba237a to 546275b Compare August 15, 2018 17:10

BridgeAR added assert Issues and PRs related to the assert subsystem. util Issues and PRs related to the built-in util module. and removed blocked PRs that are blocked by other issues or PRs. labels Aug 15, 2018

BridgeAR added the performance Issues and PRs related to the performance of Node.js. label Aug 15, 2018

BridgeAR requested review from Trott, benjamingr, jasnell and mscdex August 15, 2018 20:51

BridgeAR added dont-land-on-v6.x labels Aug 15, 2018

BridgeAR requested review from andrasq, apapirovski, devsnek and mcollina August 16, 2018 23:49

BridgeAR requested a review from ryzokuken August 16, 2018 23:49

ryzokuken approved these changes Aug 17, 2018

View reviewed changes

BridgeAR requested a review from addaleax August 17, 2018 16:23

joyeecheung mentioned this pull request Aug 17, 2018

CI Failures: 20180817 nodejs/reliability#14

Closed

addaleax reviewed Aug 19, 2018

View reviewed changes

BridgeAR added 2 commits August 19, 2018 20:30

util,assert: improve performance

6e7cf50

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.

fixup: handle c++ error properly

3c83e47

BridgeAR force-pushed the improve-comparison-further branch from 32520f6 to 3c83e47 Compare August 19, 2018 18:31

targos removed the dont-land-on-v9.x label Aug 19, 2018

addaleax approved these changes Aug 19, 2018

View reviewed changes

BridgeAR closed this Aug 20, 2018

This was referenced Aug 20, 2018

test-digitalocean-fedora27-x64-1 keeps timing out nodejs/build#1463

Closed

CI failures: 20180820 nodejs/reliability#15

Closed

bmeurer reviewed Aug 20, 2018

View reviewed changes

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

BridgeAR deleted the improve-comparison-further branch January 20, 2020 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util,assert: improve comparison performance #22197

util,assert: improve comparison performance #22197

BridgeAR commented Aug 8, 2018 •

edited

nodejs-github-bot commented Aug 8, 2018

mscdex commented Aug 8, 2018

benjamingr left a comment

BridgeAR commented Aug 9, 2018

BridgeAR commented Aug 9, 2018

BridgeAR commented Aug 15, 2018

BridgeAR commented Aug 15, 2018

BridgeAR commented Aug 15, 2018

Trott commented Aug 15, 2018 •

edited

BridgeAR commented Aug 15, 2018

BridgeAR commented Aug 17, 2018

BridgeAR commented Aug 17, 2018

benjamingr commented Aug 17, 2018

ryzokuken left a comment

ryzokuken Aug 17, 2018

BridgeAR Aug 17, 2018

BridgeAR Aug 19, 2018

ryzokuken Aug 17, 2018

BridgeAR Aug 17, 2018

ryzokuken Aug 17, 2018

joyeecheung Aug 17, 2018 •

edited

ryzokuken Aug 17, 2018

addaleax Aug 19, 2018

BridgeAR Aug 19, 2018

addaleax left a comment

addaleax Aug 19, 2018

BridgeAR commented Aug 19, 2018

Trott commented Aug 19, 2018

BridgeAR commented Aug 19, 2018

addaleax left a comment

addaleax Aug 19, 2018

BridgeAR Aug 20, 2018

BridgeAR commented Aug 20, 2018

bmeurer left a comment

util,assert: improve comparison performance #22197

util,assert: improve comparison performance #22197

Conversation

BridgeAR commented Aug 8, 2018 • edited

Checklist

nodejs-github-bot commented Aug 8, 2018

mscdex commented Aug 8, 2018

benjamingr left a comment

Choose a reason for hiding this comment

BridgeAR commented Aug 9, 2018

BridgeAR commented Aug 9, 2018

BridgeAR commented Aug 15, 2018

BridgeAR commented Aug 15, 2018

BridgeAR commented Aug 15, 2018

Trott commented Aug 15, 2018 • edited

BridgeAR commented Aug 15, 2018

BridgeAR commented Aug 17, 2018

BridgeAR commented Aug 17, 2018

benjamingr commented Aug 17, 2018

ryzokuken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Aug 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Aug 19, 2018

Trott commented Aug 19, 2018

BridgeAR commented Aug 19, 2018

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Aug 20, 2018

bmeurer left a comment

Choose a reason for hiding this comment

BridgeAR commented Aug 8, 2018 •

edited

Trott commented Aug 15, 2018 •

edited

joyeecheung Aug 17, 2018 •

edited