Skip to content
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: typed array deepequal performance fix #4330

Conversation

claudiorodriguez
Copy link
Contributor

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

Fixes: #4294

@Fishrock123 Fishrock123 added the assert Issues and PRs related to the assert subsystem. label Dec 17, 2015
@jasnell
Copy link
Member

jasnell commented Dec 17, 2015

LGTM

@@ -143,6 +143,12 @@ assert.deepStrictEqual = function deepStrictEqual(actual, expected, message) {
};

function _deepEqual(actual, expected, strict) {
// If both values are instances of typed arrays, wrap them in
// a Buffer each to increase performance
if (ArrayBuffer.isView(actual) && ArrayBuffer.isView(expected)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cjihrig
Copy link
Contributor

cjihrig commented Dec 17, 2015

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

@cjihrig
Copy link
Contributor

cjihrig commented Dec 17, 2015

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 claudiorodriguez force-pushed the assert-performance-typed-arrays branch 2 times, most recently from 83468ba to 10d83e5 Compare December 17, 2015 17:40
@claudiorodriguez
Copy link
Contributor Author

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

@@ -170,7 +170,12 @@ function _deepEqual(actual, expected, strict) {
(expected === null || typeof expected !== 'object')) {
return strict ? actual === expected : actual == expected;

// 7.5 For all other Object pairs, including Array objects, equivalence is
// 7.5. If both values are instances of typed arrays, wrap them in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

@claudiorodriguez
Copy link
Contributor Author

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

@Trott
Copy link
Member

Trott commented Dec 17, 2015

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
Copy link
Contributor Author

@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
Copy link
Member

Trott commented Dec 17, 2015

@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
Copy link
Contributor

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

@claudiorodriguez
Copy link
Contributor Author

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

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

Fixes: nodejs#4294
@claudiorodriguez
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Member

LGTM

1 similar comment
@silverwind
Copy link
Contributor

LGTM

@cjihrig
Copy link
Contributor

cjihrig commented Dec 21, 2015

silverwind pushed a commit that referenced this pull request Dec 21, 2015
assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

PR-URL: #4330
Fixes: #4294
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@silverwind
Copy link
Contributor

Thanks! Landed in 6378622.

@silverwind silverwind closed this Dec 21, 2015
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Dec 22, 2015
assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

PR-URL: nodejs#4330
Fixes: nodejs#4294
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

PR-URL: nodejs#4330
Fixes: nodejs#4294
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
* http:
  - A new status code was added: 451 - "Unavailable For Legal Reasons" (Max Barinov) nodejs#4377
  - Idle sockets that have been kept alive now handle errors (José F. Romaniello) nodejs#4482
* This release also includes several minor performance improvements:
  - assert: deepEqual is now speedier when comparing TypedArrays (Claudio Rodriguez) nodejs#4330
  - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622
  - node: Improved accessor perf of process.env (Trevor Norris) nodejs#3780
  - node: Improved performance of process.hrtime() (Trevor Norris) nodejs#3780, (Evan Lucas) nodejs#4484
  - node: Improved GetActiveHandles performance (Trevor Norris) nodejs#3780
  - util: Use faster iteration in util.format() (Jackson Tian) nodejs#3964

PR-URL: nodejs#4547
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jan 11, 2016
* http:
  - A new status code was added: 451 - "Unavailable For Legal Reasons"
(Max Barinov) nodejs#4377
  - Idle sockets that have been kept alive now handle errors (José F.
Romaniello) nodejs#4482
* This release also includes several minor performance improvements:
  - assert: deepEqual is now speedier when comparing TypedArrays
(Claudio Rodriguez) nodejs#4330
  - lib: Use arrow functions instead of bind where possible (Minwoo
Jung) nodejs#3622
  - node: Improved accessor perf of process.env (Trevor Norris)
nodejs#3780
  - node: Improved performance of process.hrtime() (Trevor Norris)
nodejs#3780, (Evan Lucas)
nodejs#4484
  - node: Improved GetActiveHandles performance (Trevor Norris)
nodejs#3780
  - util: Use faster iteration in util.format() (Jackson Tian)
nodejs#3964

Refs: nodejs#4547
PR-URL: nodejs#4623
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 13, 2016
assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

PR-URL: #4330
Fixes: #4294
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

PR-URL: #4330
Fixes: #4294
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

PR-URL: nodejs#4330
Fixes: nodejs#4294
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
* http:
  - A new status code was added: 451 - "Unavailable For Legal Reasons"
(Max Barinov) nodejs#4377
  - Idle sockets that have been kept alive now handle errors (José F.
Romaniello) nodejs#4482
* This release also includes several minor performance improvements:
  - assert: deepEqual is now speedier when comparing TypedArrays
(Claudio Rodriguez) nodejs#4330
  - lib: Use arrow functions instead of bind where possible (Minwoo
Jung) nodejs#3622
  - node: Improved accessor perf of process.env (Trevor Norris)
nodejs#3780
  - node: Improved performance of process.hrtime() (Trevor Norris)
nodejs#3780, (Evan Lucas)
nodejs#4484
  - node: Improved GetActiveHandles performance (Trevor Norris)
nodejs#3780
  - util: Use faster iteration in util.format() (Jackson Tian)
nodejs#3964

Refs: nodejs#4547
PR-URL: nodejs#4623
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants