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

Caw/improve array comparison performance #1

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+41 −29
Open

Caw/improve array comparison performance #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
686deb7
assert,util: improve array comparison
BridgeAR Mar 25, 2025
f46895a
assert,util: improve unequal number comparison performance
BridgeAR Mar 26, 2025
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
26 changes: 20 additions & 6 deletions benchmark/assert/deepequal-simple-array-and-set.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ const bench = common.createBenchmark(main, {
method: [
'deepEqual_Array',
'notDeepEqual_Array',
'deepEqual_sparseArray',
'notDeepEqual_sparseArray',
'deepEqual_Set',
'notDeepEqual_Set',
],
Expand All @@ -25,18 +27,30 @@ function run(fn, n, actual, expected) {
}

function main({ n, len, method, strict }) {
const actual = [];
const expected = [];
let actual = Array.from({ length: len }, (_, i) => i);
// Contain one undefined value to trigger a specific code path
actual[0] = undefined;
Copy link
Author

Choose a reason for hiding this comment

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

Setting actual[0] to undefined here affects all test cases, including non-sparse ones. This changes the behavior of existing tests like 'deepEqual_Array' which should test dense arrays without undefined elements. Move this setup into the sparse test cases only.

SUGGESTION:

- actual[0] = undefined;

let expected = actual.slice(0);

for (let i = 0; i < len; i++) {
actual.push(i);
expected.push(i);
}
if (method.includes('not')) {
expected[len - 1] += 1;
}

switch (method) {
case 'deepEqual_sparseArray':
case 'notDeepEqual_sparseArray':
actual = new Array(len);
for (let i = 0; i < len; i += 2) {
actual[i] = i;
}
expected = actual.slice(0);
if (method.includes('not')) {
expected[len - 2] += 1;
run(strict ? notDeepStrictEqual : notDeepEqual, n, actual, expected);
} else {
run(strict ? deepStrictEqual : deepEqual, n, actual, expected);
}
break;
case 'deepEqual_Array':
run(strict ? deepStrictEqual : deepEqual, n, actual, expected);
break;
Expand Down
1 change: 1 addition & 0 deletions lib/internal/assert/myers_diff.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ function myersDiff(actual, expected, checkCommaDisparity = false) {
const actualLength = actual.length;
const expectedLength = expected.length;
const max = actualLength + expectedLength;
// TODO(BridgeAR): Cap the input in case the values go beyond the limit of 2^31 - 1.
Copy link
Author

Choose a reason for hiding this comment

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

The TODO about capping input lengths is valid but unimplemented. Failing to cap could cause Int32Array overflow when actualLength + expectedLength exceeds ~2^31, leading to incorrect diffs or crashes. Must implement input length validation before proceeding.

SUGGESTION:

+ const MAX_SAFE_LENGTH = 0x7FFFFFF0; // ~2^31-16
+ if (actualLength > MAX_SAFE_LENGTH || expectedLength > MAX_SAFE_LENGTH) {
+   throw new Error('Input arrays too large for Myers diff');
+ }
  const actualLength = actual.length;
  const expectedLength = expected.length;
  const max = actualLength + expectedLength;
- const v = new Int32Array(2 * max + 1);
+ if (max > MAX_SAFE_LENGTH) {
+   throw new Error('Combined lengths exceed safe limit');
+ }
+ const v = new Int32Array(2 * max + 1);

const v = new Int32Array(2 * max + 1);
const trace = [];

Expand Down
43 changes: 20 additions & 23 deletions lib/internal/util/comparisons.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ const {
BooleanPrototypeValueOf,
DatePrototypeGetTime,
Error,
NumberIsNaN,
NumberPrototypeValueOf,
ObjectGetOwnPropertySymbols: getOwnSymbols,
ObjectGetPrototypeOf,
Expand Down Expand Up @@ -185,7 +184,9 @@ function innerDeepEqual(val1, val2, mode, memos) {
// Check more closely if val1 and val2 are equal.
if (mode !== kLoose) {
if (typeof val1 === 'number') {
return NumberIsNaN(val1) && NumberIsNaN(val2);
// Check for NaN
// eslint-disable-next-line no-self-compare
return val1 !== val1 && val2 !== val2;
}
if (typeof val2 !== 'object' ||
typeof val1 !== 'object' ||
Expand All @@ -196,11 +197,10 @@ function innerDeepEqual(val1, val2, mode, memos) {
}
} else {
if (val1 === null || typeof val1 !== 'object') {
if (val2 === null || typeof val2 !== 'object') {
// eslint-disable-next-line eqeqeq
return val1 == val2 || (NumberIsNaN(val1) && NumberIsNaN(val2));
}
return false;
return (val2 === null || typeof val2 !== 'object') &&
// Check for NaN
// eslint-disable-next-line eqeqeq, no-self-compare
(val1 == val2 || (val1 !== val1 && val2 !== val2));
}
if (val2 === null || typeof val2 !== 'object') {
return false;
Expand Down Expand Up @@ -384,9 +384,7 @@ function keyCheck(val1, val2, mode, memos, iterationType, keys2) {
}
} else if (keys2.length !== ObjectKeys(val1).length) {
return false;
}

if (mode === kStrict) {
} else if (mode === kStrict) {
const symbolKeysA = getOwnSymbols(val1);
if (symbolKeysA.length !== 0) {
let count = 0;
Expand Down Expand Up @@ -502,7 +500,9 @@ function findLooseMatchingPrimitives(prim) {
// a regular number and not NaN.
// Fall through
case 'number':
if (NumberIsNaN(prim)) {
// Check for NaN
// eslint-disable-next-line no-self-compare
if (prim !== prim) {
return false;
}
}
Expand Down Expand Up @@ -758,9 +758,9 @@ function sparseArrayEquiv(a, b, mode, memos, i) {
if (keysA.length !== keysB.length) {
return false;
}
for (; i < keysA.length; i++) {
const key = keysA[i];
if (!hasOwn(b, key) || !innerDeepEqual(a[key], b[key], mode, memos)) {
for (; i < keysB.length; i++) {
const key = keysB[i];
if ((a[key] === undefined && !hasOwn(a, key)) || !innerDeepEqual(a[key], b[key], mode, memos)) {
return false;
Comment on lines +761 to 764
Copy link
Author

Choose a reason for hiding this comment

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

Looping over only keysB fails to check keys present in a but not in b, leading to false positives. Both objects' keys must be fully compared to ensure equivalence.

SUGGESTION:

- for (; i < keysB.length; i++) {
+ for (const key of [...keysA, ...keysB].filter((key, index, self) => self.indexOf(key) === index))) {

}
}
Expand All @@ -782,17 +782,14 @@ function objEquiv(a, b, mode, keys2, memos, iterationType) {
return partialArrayEquiv(a, b, mode, memos);
}
for (let i = 0; i < a.length; i++) {
if (!innerDeepEqual(a[i], b[i], mode, memos)) {
return false;
}
const isSparseA = a[i] === undefined && !hasOwn(a, i);
const isSparseB = b[i] === undefined && !hasOwn(b, i);
if (isSparseA !== isSparseB) {
if (b[i] === undefined) {
if (!hasOwn(b, i))
return sparseArrayEquiv(a, b, mode, memos, i);
if (a[i] !== undefined || !hasOwn(a, i))
return false;
} else if (a[i] === undefined || !innerDeepEqual(a[i], b[i], mode, memos)) {
return false;
}
if (isSparseA) {
return sparseArrayEquiv(a, b, mode, memos, i);
}
}
} else if (iterationType === kIsSet) {
if (!setEquiv(a, b, mode, memos)) {
Expand Down