assert: improve performance - #13973
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Why not do
const tesee = assert[conf.method];Instead of the whole switch?
Sorry, something went wrong.
There was a problem hiding this comment.
Well, the values do change at least partly so we can't just remove the switch. I can combine the ones that have the same arguments if you want me to.
Sorry, something went wrong.
There was a problem hiding this comment.
Im ±0, your call.
Sorry, something went wrong.
There was a problem hiding this comment.
I think I prefer to stick to the way it is as that way it's simpler to add more tests for the same function with different inputs.
Sorry, something went wrong.
There was a problem hiding this comment.
😄
I'm gonna dig and find how did that happen. Probably an interesting story.
Sorry, something went wrong.
There was a problem hiding this comment.
Not much of a story, it has always been there, nobody notices for 8 years
4f679fd#diff-02063f30815b78f7986ee0c212140b8eR110
Sorry, something went wrong.
|
Quick sanity: https://ci.nodejs.org/job/node-test-commit-linuxone/6915/ |
Sorry, something went wrong.
There was a problem hiding this comment.
nit: unnecessary whitespace changes
Sorry, something went wrong.
There was a problem hiding this comment.
Is it preferred to not use a line break after use strict in general or would you just want to keep that as a separate concern?
Sorry, something went wrong.
|
Not a requirement for landing but helpful if someone has the inclination to run make coverage and report back: Until a few weeks ago, lib/assert.js had 100% test coverage. A refactoring resulted in it having almost 100% test coverage but not quite. It would be good to know if and how coverage stats will be changed with this PR. |
Sorry, something went wrong.
|
@Trott I checked the coverage and I was able to remove even more code paths and move another check further up. Now the coverage is back to 100%. So PTAL. Those changes are reflected in these benchmarks: Detailsassert/deepequal-prims-and-objs-big-array-set.js method="deepEqual Set" len=100000 n=25 prim="array" 42.04 % *** 6.843987e-44 assert/deepequal-prims-and-objs-big-array-set.js method="deepEqual Set" len=100000 n=25 prim="boolean" 43.57 % *** 2.703836e-46 assert/deepequal-prims-and-objs-big-array-set.js method="deepEqual Set" len=100000 n=25 prim="new-array" 39.97 % *** 4.276696e-23 assert/deepequal-prims-and-objs-big-array-set.js method="deepEqual Set" len=100000 n=25 prim="null" 41.04 % *** 2.034303e-23 assert/deepequal-prims-and-objs-big-array-set.js method="deepEqual Set" len=100000 n=25 prim="number" 42.44 % *** 5.304838e-19 assert/deepequal-prims-and-objs-big-array-set.js method="deepEqual Set" len=100000 n=25 prim="object" 39.72 % *** 3.588488e-19 assert/deepequal-prims-and-objs-big-array-set.js method="deepEqual Set" len=100000 n=25 prim="string" 52.18 % *** 9.506625e-20 assert/deepequal-prims-and-objs-big-array-set.js method="deepEqual Set" len=100000 n=25 prim="undefined" 41.51 % *** 3.533457e-24 assert/deepequal-prims-and-objs-big-array-set.js method="deepStrictEqual Set" len=100000 n=25 prim="array" 40.82 % *** 2.085361e-42 assert/deepequal-prims-and-objs-big-array-set.js method="deepStrictEqual Set" len=100000 n=25 prim="boolean" 43.76 % *** 1.154195e-44 assert/deepequal-prims-and-objs-big-array-set.js method="deepStrictEqual Set" len=100000 n=25 prim="new-array" 40.94 % *** 1.453874e-48 assert/deepequal-prims-and-objs-big-array-set.js method="deepStrictEqual Set" len=100000 n=25 prim="null" 42.73 % *** 1.021447e-36 assert/deepequal-prims-and-objs-big-array-set.js method="deepStrictEqual Set" len=100000 n=25 prim="number" 46.04 % *** 1.265156e-48 assert/deepequal-prims-and-objs-big-array-set.js method="deepStrictEqual Set" len=100000 n=25 prim="object" 40.66 % *** 1.752583e-48 assert/deepequal-prims-and-objs-big-array-set.js method="deepStrictEqual Set" len=100000 n=25 prim="string" 63.83 % *** 1.722373e-14 assert/deepequal-prims-and-objs-big-array-set.js method="deepStrictEqual Set" len=100000 n=25 prim="undefined" 42.05 % *** 7.665409e-30 assert/deepequal-prims-and-objs-big-array-set.js method="notDeepEqual Set" len=100000 n=25 prim="array" 46.48 % *** 1.125309e-42 assert/deepequal-prims-and-objs-big-array-set.js method="notDeepEqual Set" len=100000 n=25 prim="boolean" 47.13 % *** 1.913099e-42 assert/deepequal-prims-and-objs-big-array-set.js method="notDeepEqual Set" len=100000 n=25 prim="new-array" 44.68 % *** 6.298514e-28 assert/deepequal-prims-and-objs-big-array-set.js method="notDeepEqual Set" len=100000 n=25 prim="null" 51.42 % *** 8.369991e-32 assert/deepequal-prims-and-objs-big-array-set.js method="notDeepEqual Set" len=100000 n=25 prim="number" 49.55 % *** 5.140566e-41 assert/deepequal-prims-and-objs-big-array-set.js method="notDeepEqual Set" len=100000 n=25 prim="object" 46.88 % *** 5.781679e-39 assert/deepequal-prims-and-objs-big-array-set.js method="notDeepEqual Set" len=100000 n=25 prim="string" 56.48 % *** 6.461916e-20 assert/deepequal-prims-and-objs-big-array-set.js method="notDeepEqual Set" len=100000 n=25 prim="undefined" 50.79 % *** 3.532599e-27 assert/deepequal-prims-and-objs-big-array-set.js method="notDeepStrictEqual Set" len=100000 n=25 prim="array" 44.91 % *** 1.921818e-39 assert/deepequal-prims-and-objs-big-array-set.js method="notDeepStrictEqual Set" len=100000 n=25 prim="boolean" 45.89 % *** 1.467412e-25 assert/deepequal-prims-and-objs-big-array-set.js method="notDeepStrictEqual Set" len=100000 n=25 prim="new-array" 44.31 % *** 1.076166e-20 assert/deepequal-prims-and-objs-big-array-set.js method="notDeepStrictEqual Set" len=100000 n=25 prim="null" 45.83 % *** 9.819397e-26 assert/deepequal-prims-and-objs-big-array-set.js method="notDeepStrictEqual Set" len=100000 n=25 prim="number" 46.78 % *** 4.519398e-24 assert/deepequal-prims-and-objs-big-array-set.js method="notDeepStrictEqual Set" len=100000 n=25 prim="object" 45.23 % *** 2.189934e-28 assert/deepequal-prims-and-objs-big-array-set.js method="notDeepStrictEqual Set" len=100000 n=25 prim="string" 54.66 % *** 4.675293e-26 assert/deepequal-prims-and-objs-big-array-set.js method="notDeepStrictEqual Set" len=100000 n=25 prim="undefined" 58.71 % *** 5.609204e-24 assert/deepequal-prims-and-objs-big-loop.js method="deepEqual" n=1000000 prim="array" 1.36 % * 4.094796e-02 assert/deepequal-prims-and-objs-big-loop.js method="deepEqual" n=1000000 prim="boolean" -0.37 % 6.079521e-01 assert/deepequal-prims-and-objs-big-loop.js method="deepEqual" n=1000000 prim="new-array" 1.29 % 1.181016e-01 assert/deepequal-prims-and-objs-big-loop.js method="deepEqual" n=1000000 prim="null" 0.74 % 2.758807e-01 assert/deepequal-prims-and-objs-big-loop.js method="deepEqual" n=1000000 prim="number" 0.78 % 2.118160e-01 assert/deepequal-prims-and-objs-big-loop.js method="deepEqual" n=1000000 prim="object" 1.69 % * 1.241283e-02 assert/deepequal-prims-and-objs-big-loop.js method="deepEqual" n=1000000 prim="string" 0.47 % 5.299881e-01 assert/deepequal-prims-and-objs-big-loop.js method="deepEqual" n=1000000 prim="undefined" 0.41 % 5.487362e-01 assert/deepequal-prims-and-objs-big-loop.js method="deepStrictEqual" n=1000000 prim="array" 9.36 % *** 2.970686e-18 assert/deepequal-prims-and-objs-big-loop.js method="deepStrictEqual" n=1000000 prim="boolean" 8.65 % *** 1.500166e-13 assert/deepequal-prims-and-objs-big-loop.js method="deepStrictEqual" n=1000000 prim="new-array" 9.52 % *** 4.877225e-18 assert/deepequal-prims-and-objs-big-loop.js method="deepStrictEqual" n=1000000 prim="null" 10.33 % *** 7.432987e-19 assert/deepequal-prims-and-objs-big-loop.js method="deepStrictEqual" n=1000000 prim="number" 10.77 % *** 9.287721e-20 assert/deepequal-prims-and-objs-big-loop.js method="deepStrictEqual" n=1000000 prim="object" 10.81 % *** 1.189148e-20 assert/deepequal-prims-and-objs-big-loop.js method="deepStrictEqual" n=1000000 prim="string" 10.00 % *** 3.991182e-16 assert/deepequal-prims-and-objs-big-loop.js method="deepStrictEqual" n=1000000 prim="undefined" 10.00 % *** 1.348992e-18 assert/deepequal-prims-and-objs-big-loop.js method="notDeepEqual" n=1000000 prim="array" 30.07 % *** 8.674371e-34 assert/deepequal-prims-and-objs-big-loop.js method="notDeepEqual" n=1000000 prim="boolean" 1.30 % 8.945766e-02 assert/deepequal-prims-and-objs-big-loop.js method="notDeepEqual" n=1000000 prim="new-array" 32.55 % *** 3.999601e-34 assert/deepequal-prims-and-objs-big-loop.js method="notDeepEqual" n=1000000 prim="null" 1.24 % 5.040777e-02 assert/deepequal-prims-and-objs-big-loop.js method="notDeepEqual" n=1000000 prim="number" 0.85 % 2.250620e-01 assert/deepequal-prims-and-objs-big-loop.js method="notDeepEqual" n=1000000 prim="object" 28.79 % *** 9.978597e-36 assert/deepequal-prims-and-objs-big-loop.js method="notDeepEqual" n=1000000 prim="string" 0.62 % 3.536732e-01 assert/deepequal-prims-and-objs-big-loop.js method="notDeepEqual" n=1000000 prim="undefined" 1.41 % * 4.266881e-02 assert/deepequal-prims-and-objs-big-loop.js method="notDeepStrictEqual" n=1000000 prim="array" 23.41 % *** 9.407808e-29 assert/deepequal-prims-and-objs-big-loop.js method="notDeepStrictEqual" n=1000000 prim="boolean" 9.44 % *** 1.786321e-14 assert/deepequal-prims-and-objs-big-loop.js method="notDeepStrictEqual" n=1000000 prim="new-array" 23.30 % *** 8.095747e-38 assert/deepequal-prims-and-objs-big-loop.js method="notDeepStrictEqual" n=1000000 prim="null" 11.15 % *** 6.224962e-24 assert/deepequal-prims-and-objs-big-loop.js method="notDeepStrictEqual" n=1000000 prim="number" 10.23 % *** 8.406336e-23 assert/deepequal-prims-and-objs-big-loop.js method="notDeepStrictEqual" n=1000000 prim="object" 22.63 % *** 1.001633e-34 assert/deepequal-prims-and-objs-big-loop.js method="notDeepStrictEqual" n=1000000 prim="string" 10.89 % *** 6.040105e-19 assert/deepequal-prims-and-objs-big-loop.js method="notDeepStrictEqual" n=1000000 prim="undefined" 9.79 % *** 3.325113e-17 [refack: folded benchmark results] |
Sorry, something went wrong.
There was a problem hiding this comment.
@mscdex @nodejs/benchmarking Is this sort of benchmark change OK? Do we usually do changes like this, or do we more typically add additional cases (so n: [1e3, 1e5])?
Sorry, something went wrong.
There was a problem hiding this comment.
Also IMHO even 1e5 is too low for a CPU bound op.
(Some of the p values are too high)
Sorry, something went wrong.
There was a problem hiding this comment.
I'd be tempted to suggest adding as an extra - unless something has been found to be not valid with it being 1e3.
@joyeecheung implemented this benchmark in the first place, was there any particular reason for choosing 1e3 and not a larger number?
The other thing to consider is the length of time to run the benchmark, we should be careful about adding more variants which would add to longer run times for the benchmark suite
Sorry, something went wrong.
There was a problem hiding this comment.
Using different n is not really useful as far as I can tell. And as @refack pointed out using 1e5 is still a low value. Otherwise it's difficult to get a higher significance if I'm correct.
Sorry, something went wrong.
Sorry, something went wrong.
|
I stumbled open a few more tiny improvements (tiny buffers and different sized buffers are checked faster now) and turbofan still can't handle try catch well. |
Sorry, something went wrong.
There was a problem hiding this comment.
why not for?
Sorry, something went wrong.
There was a problem hiding this comment.
shrug
Sorry, something went wrong.
refack
left a comment
There was a problem hiding this comment.
% /benchmark/ changes decision
Sorry, something went wrong.
There was a problem hiding this comment.
This is a special case for compatibility so the link to the original PR should be kept IMO
Sorry, something went wrong.
There was a problem hiding this comment.
Done
Sorry, something went wrong.
There was a problem hiding this comment.
Maybe a comment about returning undefined means we need to check further?
Sorry, something went wrong.
There was a problem hiding this comment.
Done
Sorry, something went wrong.
There was a problem hiding this comment.
Maybe get rid of the space in the argument values
Sorry, something went wrong.
There was a problem hiding this comment.
Would be a underscore be fine instead? The first part is the function name and that's why I'd like some kind of distinction.
Sorry, something went wrong.
|
Pre-land CI: https://ci.nodejs.org/job/node-test-commit/10890/ |
Sorry, something went wrong.
benjamingr
left a comment
There was a problem hiding this comment.
Actual changes LGTM and make a nice improvement - since it changes a lot of code a CITGM run would also be appreciated.
Sorry, something went wrong.
|
CITGM (Base): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/897/ |
Sorry, something went wrong.
|
@BridgeAR I tried to resolve the conflicts, but it's too delicate. So please rebase. |
Sorry, something went wrong.
The benchmarks had the strict and non strict labels switched. This is fixed and the benchmarks were extended to check more possible input types and function calls.
The lazy loading is not needed as the errors themself lazy load assert. Therefore the circle is working as intended even without this lazy loading. Improve Array, object, ArrayBuffer, Set and Map performance in all deepEqual checks by removing unecessary code paths and by moving expensive checks further back. Improve throws and doesNotThrow performance by removing dead code and simplifying the overall logic.
assert.strictEqual can either have two or three arguments, not four.
|
Rebased |
Sorry, something went wrong.
Sorry, something went wrong.
The lazy loading is not needed as the errors themself lazy load assert. Therefore the circle is working as intended even without this lazy loading. Improve Array, object, ArrayBuffer, Set and Map performance in all deepEqual checks by removing unecessary code paths and by moving expensive checks further back. Improve throws and doesNotThrow performance by removing dead code and simplifying the overall logic. PR-URL: nodejs#13973 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sorry, something went wrong.
|
@refack Just for context, what’s the motivation for leaving out benchmarks? Anyway, if anything in here can be backported to v8.x, it would need to be backported manually (guide). We’re already in a situation where a lot of changes to assert depend on semver-major changes, so any work to reduce the delta would be really appreciated. |
Sorry, something went wrong.
They were done in this PR to prove it actually has a positive performance gain, but It would perturb https://benchmarking.nodejs.org/ so we spun off #14147 to be discussed just in the context of benchmarking changes. |
Sorry, something went wrong.
|
P.S. should probably land with #14258 |
Sorry, something went wrong.
|
@addaleax I am going to backport this later on. |
Sorry, something went wrong.
|
Lands cleanly now :) |
Sorry, something went wrong.
The lazy loading is not needed as the errors themself lazy load assert. Therefore the circle is working as intended even without this lazy loading. Improve Array, object, ArrayBuffer, Set and Map performance in all deepEqual checks by removing unecessary code paths and by moving expensive checks further back. Improve throws and doesNotThrow performance by removing dead code and simplifying the overall logic. PR-URL: #13973 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
While refactoring the assert in #13862 I stumbled open a couple of improvements. This change is only about performance changes and not about style.
I added a few more benchmarks and fixed the description of the old ones.
BenchmarksChecklist
Affected core subsystem(s)
assert