-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
assert: typed array deepequal performance fix #4330
Conversation
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)) { |
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.
Can this logic be pushed down further? At least past the "7.1." check on line 152.
The test should probably have some cases where the two values are not equal. |
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? |
83468ba
to
10d83e5
Compare
@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 |
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.
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.
10d83e5
to
94bb526
Compare
@Trott Oops, thanks for catching that, fixed jslint issues. |
94bb526
to
5b14d55
Compare
@Trott You really learn something new every day, thanks again. |
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. |
5b14d55
to
8f6a57c
Compare
@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 |
8f6a57c
to
f79654a
Compare
@fansworld-claudio On the benchmark file: Yep, exactly. Thanks! 👍 And yes, the |
Can we see some benchmark numbers for other primitve type? There should be a regression, hopefully small. |
@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
f79654a
to
a6a874b
Compare
@silverwind added two benchmarks: big array objects (falling into 7.5): before PR:
after PR:
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:
after PR:
Also seems to be within random variance, after running it a few times... at least running it on my computer. |
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 👍 |
LGTM |
1 similar comment
LGTM |
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 <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Thanks! Landed in 6378622. |
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 <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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 <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* 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
* 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 <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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 <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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 <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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 <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* 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 <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.
Fixes: #4294