-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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,util: improve comparison performance #46593
Conversation
588b4de
to
64eb998
Compare
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.
lgtm. thanks for the mention. always a pleasure to review a performance pull request
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.
lgtm
What's the semver-major part of the change (so we can explain it in the v20 changelog)? |
A small correctness patch is included that causes recursions to This passed so far and is now going to fail: const a = {};
a.a = a;
const b = {};
b.a = {};
b.a.a = a;
assert.deepStrictEqual(a, b); |
64eb998
to
c0a82bd
Compare
I included one more commit that actually reverts the breaking change. This allows to backport the overall PR and to include the fix right afterwards as semver major without a hassle. |
c0a82bd
to
13482c6
Compare
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.
lgtm
@nodejs/node-core-utils does anyone know why the commit-queue is not triggered with the applied labels? Is there maybe a trick to get the commit-queue to run for this PR that I missed? |
See https://github.com/nodejs/node/actions/runs/4223308992/jobs/7332895355
|
@targos seems like the detection solely relies upon the reported state here and that is not always correct. |
This reworks most assert benchmarks to provide more reliable test cases that also test more cases than before while keeping the runtime low. Signed-off-by: Ruben Bridgewater <[email protected]> PR-URL: nodejs#46593 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
These tests are a copy of the assert tests that verify the deep equal comparison algorithm. There is no need to duplicate the tests as changing one would require to change the tests in two places without knowing which ones to actually rely upon. Signed-off-by: Ruben Bridgewater <[email protected]> PR-URL: nodejs#46593 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
This is mainly a performance improvement for a lot of simple cases. Diverging elements are detected earlier and equal entries are partially also detected faster. A small correctness patch is also included where recursions now stop as soon as either side has a circular structure. Before, both sides had to have a circular structure at the specific comparison which could have caused more checks that likely fail at a later point. Signed-off-by: Ruben Bridgewater <[email protected]> PR-URL: nodejs#46593 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
This commit is there to be reverted after merging. It makes it easy to backport the overall PR and allows easy forward fixing. Signed-off-by: Ruben Bridgewater <[email protected]> PR-URL: nodejs#46593 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
13482c6
to
575784b
Compare
Landed in 1d5058d...575784b |
This is not landing cleanly on v18.x, would lyo umind to rebase? |
This reworks most assert benchmarks to provide more reliable test cases that also test more cases than before while keeping the runtime low. Signed-off-by: Ruben Bridgewater <[email protected]> PR-URL: #46593 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
These tests are a copy of the assert tests that verify the deep equal comparison algorithm. There is no need to duplicate the tests as changing one would require to change the tests in two places without knowing which ones to actually rely upon. Signed-off-by: Ruben Bridgewater <[email protected]> PR-URL: #46593 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
This is mainly a performance improvement for a lot of simple cases. Diverging elements are detected earlier and equal entries are partially also detected faster. A small correctness patch is also included where recursions now stop as soon as either side has a circular structure. Before, both sides had to have a circular structure at the specific comparison which could have caused more checks that likely fail at a later point. Signed-off-by: Ruben Bridgewater <[email protected]> PR-URL: #46593 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
This commit is there to be reverted after merging. It makes it easy to backport the overall PR and allows easy forward fixing. Signed-off-by: Ruben Bridgewater <[email protected]> PR-URL: #46593 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
I believe this PR may have caused a regression from v18 to v20. const x = ['x'];
assert.deepStrictEqual(new Set([x, ['y']]), new Set([x, ['y']]));
The bug seems to occur only when comparing Looking at the changed code, a new "SafeSet" is created only containing the objects in |
benchmark: rework assert benchmarks for correctness
test: remove obsolete util.isDeepStrictEqual tests
assert,util: improve deep equal comparison performance
Signed-off-by: Ruben Bridgewater [email protected]
Benchmark