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.

Sign up for GitHub

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: fix loose set and map comparison #22495

Closed
wants to merge 3 commits into from
Closed

assert: fix loose set and map comparison #22495

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
63ea875
assert: fix loose set and map comparison
BridgeAR Aug 23, 2018
e80c545
fixup: address comment
BridgeAR Aug 29, 2018
c030f42
fixup
BridgeAR Aug 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
177 changes: 73 additions & 104 deletions lib/internal/util/comparisons.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,23 +374,48 @@ function setHasEqualElement(set, val1, strict, memo) {
return false;
}

// Note: we currently run this multiple times for each loose key!
// This is done to prevent slowing down the average case.
function setHasLoosePrim(a, b, val) {
const altValues = findLooseMatchingPrimitives(val);
if (altValues === undefined)
return false;
// See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness#Loose_equality_using
// Sadly it is not possible to detect corresponding values properly in case the
// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@BridgeAR BridgeAR Aug 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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%

Copy link
Member Author

@BridgeAR BridgeAR Aug 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

case 'undefined':
return null;
case 'object': // Only pass in null as object!
return undefined;
case 'symbol':
return false;
case 'string':
const number = +prim;
if (Number.isNaN(number)) {
return false;
Copy link
Member

@jdalton jdalton Aug 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

☝️ might pluck the Number.isNaN reference above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment addressed.

}
}
return true;
}

let matches = 1;
for (var i = 0; i < altValues.length; i++) {
if (b.has(altValues[i])) {
matches--;
}
if (a.has(altValues[i])) {
matches++;
}
function setMightHaveLoosePrim(a, b, prim) {
const altValue = findLooseMatchingPrimitives(prim);
if (altValue != null)
return altValue;

return b.has(altValue) && !a.has(altValue);
}

function mapMightHaveLoosePrim(a, b, prim, item, memo) {
const altValue = findLooseMatchingPrimitives(prim);
if (altValue != null) {
return altValue;
}
const curB = b.get(altValue);
if (curB === undefined && !b.has(altValue) ||
!innerDeepEqual(item, curB, false, memo)) {
return false;
}
return matches === 0;
const curA = a.get(altValue);
return curA === undefined && a.has(altValue) ||
innerDeepEqual(item, curA, false, memo);
}

function setEquiv(a, b, strict, memo) {
Expand All @@ -410,8 +435,19 @@ function setEquiv(a, b, strict, memo) {
// hunting for something thats deep-(strict-)equal to it. To make this
// O(n log n) complexity we have to copy these values in a new set first.
set.add(val);
} else if (!b.has(val) && (strict || !setHasLoosePrim(a, b, val))) {
return false;
} else if (!b.has(val)) {
if (strict)
return false;

// Fast path to detect missing string, symbol, undefined and null values.
if (!setMightHaveLoosePrim(a, b, val)) {
return false;
}

if (set === null) {
set = new Set();
}
set.add(val);
}
}

Expand All @@ -422,96 +458,18 @@ function setEquiv(a, b, strict, memo) {
if (typeof val === 'object' && val !== null) {
if (!setHasEqualElement(set, val, strict, memo))
return false;
} else if (!a.has(val) && (strict || !setHasLoosePrim(b, a, val))) {
} else if (!strict &&
!a.has(val) &&
!setHasEqualElement(set, val, strict, memo)) {
return false;
}
}
return set.size === 0;
}

return true;
}

// See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness#Loose_equality_using
function findLooseMatchingPrimitives(prim) {
switch (typeof prim) {
case 'number':
if (prim === 0) {
return ['', '0', false];
}
if (prim === 1) {
return ['1', true];
}
return ['' + prim];
case 'string':
if (prim === '' || prim === '0') {
return [0, false];
}
if (prim === '1') {
return [1, true];
}
const number = +prim;
if ('' + number === prim) {
return [number];
}
return;
case 'undefined':
return [null];
case 'object': // Only pass in null as object!
return [undefined];
case 'boolean':
if (prim === false) {
return ['', '0', 0];
}
return ['1', 1];
}
}

// This is a ugly but relatively fast way to determine if a loose equal entry
// currently has a correspondent matching entry. Otherwise checking for such
// values would be way more expensive (O(n^2)).
// Note: we currently run this multiple times for each loose key!
// This is done to prevent slowing down the average case.
function mapHasLoosePrim(a, b, key1, memo, item1, item2) {
const altKeys = findLooseMatchingPrimitives(key1);
if (altKeys === undefined)
return false;

const setA = new Set();
const setB = new Set();

let keyCount = 1;

setA.add(item1);
if (b.has(key1)) {
keyCount--;
setB.add(item2);
}

for (var i = 0; i < altKeys.length; i++) {
const key2 = altKeys[i];
if (a.has(key2)) {
keyCount++;
setA.add(a.get(key2));
}
if (b.has(key2)) {
keyCount--;
setB.add(b.get(key2));
}
}
if (keyCount !== 0 || setA.size !== setB.size)
return false;

for (const val of setA) {
if (typeof val === 'object' && val !== null) {
if (!setHasEqualElement(setB, val, false, memo))
return false;
} else if (!setB.has(val) && !setHasLoosePrim(setA, setB, val)) {
return false;
}
}
return true;
}

function mapHasEqualEntry(set, map, key1, item1, strict, memo) {
// To be able to handle cases like:
// Map([[{}, 'a'], [{}, 'b']]) vs Map([[{}, 'b'], [{}, 'a']])
Expand Down Expand Up @@ -541,9 +499,17 @@ function mapEquiv(a, b, strict, memo) {
// almost all possible cases.
const item2 = b.get(key);
if ((item2 === undefined && !b.has(key) ||
!innerDeepEqual(item1, item2, strict, memo)) &&
(strict || !mapHasLoosePrim(a, b, key, memo, item1, item2))) {
return false;
!innerDeepEqual(item1, item2, strict, memo))) {
if (strict)
return false;
// Fast path to detect missing string, symbol, undefined and null
// keys.
if (!mapMightHaveLoosePrim(a, b, key, item1, memo))
return false;
if (set === null) {
set = new Set();
}
set.add(key);
}
}
}
Expand All @@ -553,11 +519,14 @@ function mapEquiv(a, b, strict, memo) {
if (typeof key === 'object' && key !== null) {
if (!mapHasEqualEntry(set, a, key, item, strict, memo))
return false;
} else if (!a.has(key) &&
(strict || !mapHasLoosePrim(b, a, key, memo, item))) {
} else if (!strict &&
(!a.has(key) ||
!innerDeepEqual(a.get(key), item, false, memo)) &&
!mapHasEqualEntry(set, a, key, item, false, memo)) {
return false;
}
}
return set.size === 0;
}

return true;
Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-assert-deep.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,13 +369,13 @@ assertDeepAndStrictEqual(
new Map([[null, 3]])
);
assertOnlyDeepEqual(
new Map([[null, undefined]]),
new Map([[undefined, null]])
new Map([[undefined, null], ['+000', 2n]]),
new Map([[null, undefined], [false, '2']]),
);

assertOnlyDeepEqual(
new Set([null, '']),
new Set([undefined, 0])
new Set([null, '', 1n, 5, 2n, false]),
new Set([undefined, 0, 5n, true, '2', '-000'])
);
assertNotDeepOrStrict(
new Set(['']),
Expand Down