assert: fix loose set and map comparison #22495

BridgeAR · 2018-08-24T00:38:15Z

The fast path did not anticipate different ways to express a loose
equal string value (e.g., 1n == '+0001'). This is now fixed with the
downside that all primitives that could theoretically have equal
entries must go through a full comparison.

Only strings (partially), symbols, undefined and null can be detected
in a fast path as those entries have a strictly limited set of possible
equal entries.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

The fast path did not anticipate different ways to express a loose equal string value (e.g., 1n == '+0001'). This is now fixed with the downside that all primitives that could theoretically have equal entries must go through a full comparison. Only strings, symbols, undefined and null can be detected in a fast path as those entries have a strictly limited set of possible equal entries.

nodejs-github-bot · 2018-08-24T00:38:16Z

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/655/pipeline

mscdex · 2018-08-24T00:45:07Z

lib/internal/util/comparisons.js

+// type is a string, number, bigint or boolean. The reason is that those values
+// can match lots of different string values (e.g., 1n == '+00001').
+function findLooseMatchingPrimitives(prim) {
+  switch (typeof prim) {


Are you sure this doesn't cause performance issues still?

It does. Significantly for loose comparison for any keys that are primitives that are not null, undefined, symbols and strings that are not loosely equal to any other values.

For strings as primitives that are not loosely equal to numbers:
(A small performance increase)

assert/deepequal-set.js method='deepEqual_mixed' strict=0 len=500 n=500 -0.38 % ±0.93% ±1.24% ±1.62% assert/deepequal-set.js method='deepEqual_mixed' strict=1 len=500 n=500 *** 5.23 % ±1.89% ±2.53% ±3.30% assert/deepequal-set.js method='deepEqual_objectOnly' strict=0 len=500 n=500 -0.32 % ±1.25% ±1.67% ±2.19% assert/deepequal-set.js method='deepEqual_objectOnly' strict=1 len=500 n=500 0.89 % ±2.38% ±3.19% ±4.22% assert/deepequal-set.js method='deepEqual_primitiveOnly' strict=0 len=500 n=500 -0.05 % ±1.81% ±2.41% ±3.14% assert/deepequal-set.js method='deepEqual_primitiveOnly' strict=1 len=500 n=500 -0.04 % ±2.02% ±2.70% ±3.53% assert/deepequal-set.js method='notDeepEqual_mixed' strict=0 len=500 n=500 *** 2.59 % ±0.99% ±1.32% ±1.72% assert/deepequal-set.js method='notDeepEqual_mixed' strict=1 len=500 n=500 2.00 % ±2.38% ±3.17% ±4.12% assert/deepequal-set.js method='notDeepEqual_objectOnly' strict=0 len=500 n=500 -0.25 % ±0.84% ±1.12% ±1.46% assert/deepequal-set.js method='notDeepEqual_objectOnly' strict=1 len=500 n=500 -0.34 % ±2.01% ±2.68% ±3.49% assert/deepequal-set.js method='notDeepEqual_primitiveOnly' strict=0 len=500 n=500 *** 4.24 % ±1.89% ±2.52% ±3.28% assert/deepequal-set.js method='notDeepEqual_primitiveOnly' strict=1 len=500 n=500 0.57 % ±3.65% ±4.87% ±6.37%

For numbers as primitives:
(A significant performance loss for loose not equal checks)

assert/deepequal-set.js method='deepEqual_mixed' strict=0 len=500 n=500 * -4.01 % ±3.39% ±4.60% ±6.18% assert/deepequal-set.js method='deepEqual_mixed' strict=1 len=500 n=500 *** 4.35 % ±1.94% ±2.61% ±3.44% assert/deepequal-set.js method='deepEqual_primitiveOnly' strict=0 len=500 n=500 * 5.06 % ±4.38% ±5.93% ±7.91% assert/deepequal-set.js method='deepEqual_primitiveOnly' strict=1 len=500 n=500 0.48 % ±5.28% ±7.08% ±9.31% assert/deepequal-set.js method='notDeepEqual_mixed' strict=0 len=500 n=500 *** -87.74 % ±3.30% ±4.49% ±6.05% assert/deepequal-set.js method='notDeepEqual_mixed' strict=1 len=500 n=500 -0.52 % ±2.56% ±3.44% ±4.55% assert/deepequal-set.js method='notDeepEqual_primitiveOnly' strict=0 len=500 n=500 *** -39.88 % ±4.70% ±6.31% ±8.34% assert/deepequal-set.js method='notDeepEqual_primitiveOnly' strict=1 len=500 n=500 -2.68 % ±3.50% ±4.72% ±6.28%

I tried another approach to overcome the downside but it is simply not possible to absolutely be sure there is no other loosely equal entry.

Now a primitive that could match something else has to go through all entries at least once. Before, it would stop when the entry was found as not having a corresponding entry.

What I was referring to was specifically the use of switch (typeof prim) vs. an if-else ladder. I'm thinking V8 might still not optimize well when typeof is used in this way, because it's being treated as a variable instead of a direct comparison?

Tracking issue: https://bugs.chromium.org/p/v8/issues/detail?id=8093

Seems like there is a tiny difference. I don't think it's significant enough that we should refactor the code. Instead, V8 should just improve it and we'll benefit from it as soon as that lands in Node.

BridgeAR · 2018-08-26T11:56:55Z

@nodejs/util PTAL

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

benjamingr

I totally missed this in the original!

I'm also not a fan of the regression but given where we started and the fact this is a bug fix I recommend we land this asap and talk optimizing later.

BridgeAR · 2018-08-26T18:39:13Z

@benjamingr this code is in core since 8.x.

After thinking about it again it's likely possible by using the former approach in a similar way:

Check for "typical" loose equal entries if no matching one is found and make sure the entry is not already in the cache (primitives may only exist once in a set / as map key). If one exists, cache it. If none exists, search the whole other set / map for the entry. If none exist, fail. Otherwise, add it to the cache and continue.

This approach allows a performance nearer to the original one in all "simple" cases (1 == true) and is worse if all entries are weird string numbers (e.g., '-0000.0' == 0). However, I am not convinced to add a fast path for the loose equality as it's still fast enough and no one should use it anyway. The implementation would just be more complex.

jdalton

Reviewing less on implementation and more on philosophy. I'm 👍 on fixing the bug first then following up on perf because getting the wrong result, but fast isn't helpful. In the context of comparisons I can also see this case, loose map/set comparisons, carrying a reasonable expectation of being less speedy than more strict forms.

jdalton · 2018-08-27T03:59:22Z

lib/internal/util/comparisons.js

+    case 'string':
+      const number = +prim;
+      if (Number.isNaN(number)) {
+        return false;


☝️ might pluck the Number.isNaN reference above.

Comment addressed.

BridgeAR · 2018-08-29T12:33:57Z

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

BridgeAR · 2018-08-31T13:34:36Z

New CI https://ci.nodejs.org/job/node-test-pull-request/16894/

benjamingr · 2018-08-31T13:36:34Z

lib/internal/util/comparisons.js

@@ -387,12 +387,10 @@ function findLooseMatchingPrimitives(prim) {
    case 'symbol':
      return false;
    case 'string':
-      const number = +prim;
+      prim = +prim;


Not a huge fan of this change (makes it harder to follow IMO) but still LGTM on the PR.

Shall I change it back?

No strong feelings - I'm just not a fan of this sort of assignment since it takes another extra step to follow - but you can absolutely land as is if you want.

The fast path did not anticipate different ways to express a loose equal string value (e.g., 1n == '+0001'). This is now fixed with the downside that all primitives that could theoretically have equal entries must go through a full comparison. Only some strings, symbols, undefined, null and NaN can be detected in a fast path as those entries have a strictly limited set of possible equal entries. PR-URL: nodejs#22495 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>

BridgeAR · 2018-09-04T23:50:38Z

Landed in be5e396 🎉

The fast path did not anticipate different ways to express a loose equal string value (e.g., 1n == '+0001'). This is now fixed with the downside that all primitives that could theoretically have equal entries must go through a full comparison. Only some strings, symbols, undefined, null and NaN can be detected in a fast path as those entries have a strictly limited set of possible equal entries. PR-URL: #22495 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>

nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Aug 24, 2018

mscdex reviewed Aug 24, 2018

View reviewed changes

BridgeAR requested review from Trott, benjamingr, jdalton, mcollina, mscdex and jasnell August 26, 2018 11:58

benjamingr approved these changes Aug 26, 2018

View reviewed changes

Trott approved these changes Aug 26, 2018

View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 26, 2018

jdalton approved these changes Aug 27, 2018

View reviewed changes

jdalton reviewed Aug 27, 2018

View reviewed changes

fixup: address comment

e80c545

fixup

c030f42

benjamingr reviewed Aug 31, 2018

View reviewed changes

BridgeAR closed this Sep 4, 2018

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

beevelop mentioned this pull request Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

beevelop mentioned this pull request Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

BridgeAR deleted the support-big-int branch January 20, 2020 11:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: fix loose set and map comparison #22495

assert: fix loose set and map comparison #22495

BridgeAR commented Aug 24, 2018

nodejs-github-bot commented Aug 24, 2018

mscdex Aug 24, 2018

BridgeAR Aug 24, 2018 •

edited

Loading

BridgeAR Aug 24, 2018 •

edited

Loading

mscdex Aug 27, 2018

BridgeAR Aug 27, 2018

BridgeAR Aug 27, 2018

BridgeAR commented Aug 26, 2018

benjamingr left a comment

BridgeAR commented Aug 26, 2018

jdalton left a comment

jdalton Aug 27, 2018 •

edited

Loading

BridgeAR Aug 29, 2018

BridgeAR commented Aug 29, 2018

BridgeAR commented Aug 31, 2018

benjamingr Aug 31, 2018

BridgeAR Sep 3, 2018

benjamingr Sep 4, 2018

BridgeAR commented Sep 4, 2018

assert: fix loose set and map comparison #22495

assert: fix loose set and map comparison #22495

Conversation

BridgeAR commented Aug 24, 2018

Checklist

nodejs-github-bot commented Aug 24, 2018

Choose a reason for hiding this comment

BridgeAR Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

BridgeAR Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Aug 26, 2018

benjamingr left a comment

Choose a reason for hiding this comment

BridgeAR commented Aug 26, 2018

jdalton left a comment

Choose a reason for hiding this comment

jdalton Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Aug 29, 2018

BridgeAR commented Aug 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Sep 4, 2018

BridgeAR Aug 24, 2018 •

edited

Loading

BridgeAR Aug 24, 2018 •

edited

Loading

jdalton Aug 27, 2018 •

edited

Loading