assert: typed array deepequal performance fix

[claudiorodriguez]

assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

Fixes: #4294

[jasnell]

LGTM

[cjihrig]

Can this logic be pushed down further? At least past the "7.1." check on line 152.

[cjihrig]

The test should probably have some cases where the two values are not equal.

[cjihrig]

This seems fine, but I'd like feedback from other collaborators. Are there any edge cases where this change could come back to bite us?

[claudiorodriguez]

@cjihrig Thanks, just pushed the changes, +1 on waiting for feedback

[Trott]

These numbers (7.5) are not arbitrary! They come from the CommonJS standard. Looks like they may already be goobered up, but that's not a reason to make them further-removed from the standard they are intended to refer to.

[claudiorodriguez]

@Trott Oops, thanks for catching that, fixed jslint issues.

[claudiorodriguez]

@Trott You really learn something new every day, thanks again.

[Trott]

Is it worth taking a benchmark to confirm the performance improvement? A benchmark that we could run across different operating systems and hardware on the CI server might be useful.

[claudiorodriguez]

@Trott you mean something like what I just pushed? Kinda copied over another benchmark's code. Comparison between assert with the fix and without it shows a massive improvement. (20 vs 0.08)

PS: jslint wasn't happy about the benchmark, in fact it isn't happy about a lot of files in the benchmark folder... I think benchmarks are a tad obsolete in general

[Trott]

@fansworld-claudio On the benchmark file: Yep, exactly. Thanks! 👍

And yes, the benchmark directory is not included in make jslint so, yeah, I guess it's not surprising that there's lots of stuff in there that isn't compliant with other parts of the code base. (Currently, make jslint lints src, lib, test, and tools/eslint-rules only.)

[silverwind]

Can we see some benchmark numbers for other primitve type? There should be a regression, hopefully small.

[claudiorodriguez]

@silverwind Good idea, I'll have it tomorrow. There should only be a very slight impact when asserting inside huge loops.

[claudiorodriguez]

@silverwind added two benchmarks:

big array objects (falling into 7.5):

before PR:

assert\deepequal-prims-and-objs-big-array.js prim=null n=25: 1.14118
assert\deepequal-prims-and-objs-big-array.js prim=undefined n=25: 1.16831
assert\deepequal-prims-and-objs-big-array.js prim=a n=25: 1.21148
assert\deepequal-prims-and-objs-big-array.js prim=1 n=25: 1.22213
assert\deepequal-prims-and-objs-big-array.js prim=true n=25: 1.14505
assert\deepequal-prims-and-objs-big-array.js prim=[object Object] n=25: 1.22749
assert\deepequal-prims-and-objs-big-array.js prim=1,2,3 n=25: 1.23757
assert\deepequal-prims-and-objs-big-array.js prim=1,2,3 n=25: 1.20910

after PR:

assert\deepequal-prims-and-objs-big-array.js prim=null n=25: 1.22064
assert\deepequal-prims-and-objs-big-array.js prim=undefined n=25: 1.20545
assert\deepequal-prims-and-objs-big-array.js prim=a n=25: 1.19943
assert\deepequal-prims-and-objs-big-array.js prim=1 n=25: 1.21435
assert\deepequal-prims-and-objs-big-array.js prim=true n=25: 1.14204
assert\deepequal-prims-and-objs-big-array.js prim=[object Object] n=25: 1.18320
assert\deepequal-prims-and-objs-big-array.js prim=1,2,3 n=25: 1.19306
assert\deepequal-prims-and-objs-big-array.js prim=1,2,3 n=25: 1.21798

The fluctuations seem to be within random variance (in my computer's case), so there doesn't seem to be a real change here.

---

big loops (forcing into 7.5 with array object):

before PR:

assert\deepequal-prims-and-objs-big-loop.js prim=null n=100000: 31422.79244
assert\deepequal-prims-and-objs-big-loop.js prim=undefined n=100000: 28421.44023
assert\deepequal-prims-and-objs-big-loop.js prim=a n=100000: 30207.53243
assert\deepequal-prims-and-objs-big-loop.js prim=1 n=100000: 29245.98661
assert\deepequal-prims-and-objs-big-loop.js prim=true n=100000: 25392.93322
assert\deepequal-prims-and-objs-big-loop.js prim=[object Object] n=100000: 29751.57255
assert\deepequal-prims-and-objs-big-loop.js prim=1,2,3 n=100000: 29663.82391
assert\deepequal-prims-and-objs-big-loop.js prim=1,2,3 n=100000: 32222.55574

after PR:

assert\deepequal-prims-and-objs-big-loop.js prim=null n=100000: 28553.61637
assert\deepequal-prims-and-objs-big-loop.js prim=undefined n=100000: 30250.17099
assert\deepequal-prims-and-objs-big-loop.js prim=a n=100000: 31414.15124
assert\deepequal-prims-and-objs-big-loop.js prim=1 n=100000: 31509.91023
assert\deepequal-prims-and-objs-big-loop.js prim=true n=100000: 31548.13352
assert\deepequal-prims-and-objs-big-loop.js prim=[object Object] n=100000: 31572.05442
assert\deepequal-prims-and-objs-big-loop.js prim=1,2,3 n=100000: 31233.08257
assert\deepequal-prims-and-objs-big-loop.js prim=1,2,3 n=100000: 31618.14617

Also seems to be within random variance, after running it a few times... at least running it on my computer.

[silverwind]

Yeah, looks like v8 is optimizing the extra if branch pretty well and your results look to be just fluctuations. Thanks for adding these benchmarks 👍

[mcollina]

LGTM

[silverwind]

LGTM

[cjihrig]

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

[silverwind]

Thanks! Landed in 6378622.