From 588b4de849b4ee7689ba33f84f0e2fa70e2f71f4 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Thu, 9 Feb 2023 22:12:25 +0100
Subject: [PATCH] assert,util: improve deep equal comparison performance

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 <ruben@bridgewater.de>
---
 benchmark/assert/deepequal-object.js |   3 +-
 doc/api/assert.md                    |  12 +-
 lib/internal/util/comparisons.js     | 268 ++++++++++++++-------------
 test/parallel/test-assert-deep.js    |  22 ++-
 4 files changed, 173 insertions(+), 132 deletions(-)

diff --git a/benchmark/assert/deepequal-object.js b/benchmark/assert/deepequal-object.js
index 80bb50f7982529..4238129b292b45 100644
--- a/benchmark/assert/deepequal-object.js
+++ b/benchmark/assert/deepequal-object.js
@@ -12,7 +12,8 @@ const bench = common.createBenchmark(main, {
   combinationFilter: (p) => {
     return p.size === 1e4 && p.n === 25 ||
            p.size === 1e3 && p.n === 2e2 ||
-           p.size === 1e2 && p.n === 2e3;
+           p.size === 1e2 && p.n === 2e3 ||
+           p.size === 1;
   },
 });
 
diff --git a/doc/api/assert.md b/doc/api/assert.md
index d40300cc3fdb1b..34107c7730fdd9 100644
--- a/doc/api/assert.md
+++ b/doc/api/assert.md
@@ -546,6 +546,10 @@ An alias of [`assert.ok()`][].
 <!-- YAML
 added: v0.1.21
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/46593
+    description: Recursion now stops when either side encounters a circular
+                 reference.
   - version: v18.0.0
     pr-url: https://github.com/nodejs/node/pull/41020
     description: Regular expressions lastIndex property is now compared as well.
@@ -617,7 +621,7 @@ are also recursively evaluated by the following rules.
 * [Object wrappers][] are compared both as objects and unwrapped values.
 * `Object` properties are compared unordered.
 * [`Map`][] keys and [`Set`][] items are compared unordered.
-* Recursion stops when both sides differ or both sides encounter a circular
+* Recursion stops when both sides differ or either side encounters a circular
   reference.
 * Implementation does not test the [`[[Prototype]]`][prototype-spec] of
   objects.
@@ -727,6 +731,10 @@ parameter is an instance of an [`Error`][] then it will be thrown instead of the
 <!-- YAML
 added: v1.2.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/46593
+    description: Recursion now stops when either side encounters a circular
+                 reference.
   - version: v18.0.0
     pr-url: https://github.com/nodejs/node/pull/41020
     description: Regular expressions lastIndex property is now compared as well.
@@ -780,7 +788,7 @@ are recursively evaluated also by the following rules.
 * [Object wrappers][] are compared both as objects and unwrapped values.
 * `Object` properties are compared unordered.
 * [`Map`][] keys and [`Set`][] items are compared unordered.
-* Recursion stops when both sides differ or both sides encounter a circular
+* Recursion stops when both sides differ or either side encounters a circular
   reference.
 * [`WeakMap`][] and [`WeakSet`][] comparison does not rely on their values. See
   below for further details.
diff --git a/lib/internal/util/comparisons.js b/lib/internal/util/comparisons.js
index e41fcf2daf92a0..f2fa50df72cd44 100644
--- a/lib/internal/util/comparisons.js
+++ b/lib/internal/util/comparisons.js
@@ -17,7 +17,6 @@ const {
   ObjectPrototypeHasOwnProperty,
   ObjectPrototypePropertyIsEnumerable,
   ObjectPrototypeToString,
-  SafeMap,
   SafeSet,
   StringPrototypeValueOf,
   SymbolPrototypeValueOf,
@@ -134,21 +133,19 @@ function isEqualBoxedPrimitive(val1, val2) {
 function innerDeepEqual(val1, val2, strict, memos) {
   // All identical values are equivalent, as determined by ===.
   if (val1 === val2) {
-    if (val1 !== 0)
-      return true;
-    return strict ? ObjectIs(val1, val2) : true;
+    return val1 !== 0 || !strict || ObjectIs(val1, val2);
   }
 
   // Check more closely if val1 and val2 are equal.
   if (strict) {
-    if (typeof val1 !== 'object') {
-      return typeof val1 === 'number' && NumberIsNaN(val1) &&
-        NumberIsNaN(val2);
-    }
-    if (typeof val2 !== 'object' || val1 === null || val2 === null) {
-      return false;
-    }
-    if (ObjectGetPrototypeOf(val1) !== ObjectGetPrototypeOf(val2)) {
+    if (typeof val1 === 'number') {
+      return NumberIsNaN(val1) && NumberIsNaN(val2);
+    }
+    if (typeof val2 !== 'object' ||
+        typeof val1 !== 'object' ||
+        val1 === null ||
+        val2 === null ||
+        ObjectGetPrototypeOf(val1) !== ObjectGetPrototypeOf(val2)) {
       return false;
     }
   } else {
@@ -193,14 +190,6 @@ function innerDeepEqual(val1, val2, strict, memos) {
     if (!isRegExp(val2) || !areSimilarRegExps(val1, val2)) {
       return false;
     }
-  } else if (isNativeError(val1) || val1 instanceof Error) {
-    // Do not compare the stack as it might differ even though the error itself
-    // is otherwise identical.
-    if ((!isNativeError(val2) && !(val2 instanceof Error)) ||
-        val1.message !== val2.message ||
-        val1.name !== val2.name) {
-      return false;
-    }
   } else if (isArrayBufferView(val1)) {
     if (TypedArrayPrototypeGetSymbolToStringTag(val1) !==
         TypedArrayPrototypeGetSymbolToStringTag(val2)) {
@@ -237,6 +226,14 @@ function innerDeepEqual(val1, val2, strict, memos) {
     if (!isAnyArrayBuffer(val2) || !areEqualArrayBuffers(val1, val2)) {
       return false;
     }
+  } else if (isNativeError(val1) || val1 instanceof Error) {
+    // Do not compare the stack as it might differ even though the error itself
+    // is otherwise identical.
+    if ((!isNativeError(val2) && !(val2 instanceof Error)) ||
+        val1.message !== val2.message ||
+        val1.name !== val2.name) {
+      return false;
+    }
   } else if (isBoxedPrimitive(val1)) {
     if (!isEqualBoxedPrimitive(val1, val2)) {
       return false;
@@ -271,50 +268,53 @@ function keyCheck(val1, val2, strict, memos, iterationType, aKeys) {
   // c) Equivalent values for every corresponding key/index
   // d) For Sets and Maps, equal contents
   // Note: this accounts for both named and indexed properties on Arrays.
-  if (arguments.length === 5) {
+  const isArrayLikeObject = aKeys !== undefined;
+
+  if (aKeys === undefined) {
     aKeys = ObjectKeys(val1);
-    const bKeys = ObjectKeys(val2);
+  }
 
-    // The pair must have the same number of owned properties.
-    if (aKeys.length !== bKeys.length) {
-      return false;
+  // Cheap key test
+  if (aKeys.length > 0) {
+    for (const key of aKeys) {
+      if (!ObjectPrototypePropertyIsEnumerable(val2, key)) {
+        return false;
+      }
     }
   }
 
-  // Cheap key test
-  let i = 0;
-  for (; i < aKeys.length; i++) {
-    if (!ObjectPrototypePropertyIsEnumerable(val2, aKeys[i])) {
+  if (!isArrayLikeObject) {
+    // The pair must have the same number of owned properties.
+    if (aKeys.length !== ObjectKeys(val2).length) {
       return false;
     }
-  }
 
-  if (strict && arguments.length === 5) {
-    const symbolKeysA = ObjectGetOwnPropertySymbols(val1);
-    if (symbolKeysA.length !== 0) {
-      let count = 0;
-      for (i = 0; i < symbolKeysA.length; i++) {
-        const key = symbolKeysA[i];
-        if (ObjectPrototypePropertyIsEnumerable(val1, key)) {
-          if (!ObjectPrototypePropertyIsEnumerable(val2, key)) {
+    if (strict) {
+      const symbolKeysA = ObjectGetOwnPropertySymbols(val1);
+      if (symbolKeysA.length !== 0) {
+        let count = 0;
+        for (const key of symbolKeysA) {
+          if (ObjectPrototypePropertyIsEnumerable(val1, key)) {
+            if (!ObjectPrototypePropertyIsEnumerable(val2, key)) {
+              return false;
+            }
+            ArrayPrototypePush(aKeys, key);
+            count++;
+          } else if (ObjectPrototypePropertyIsEnumerable(val2, key)) {
             return false;
           }
-          ArrayPrototypePush(aKeys, key);
-          count++;
-        } else if (ObjectPrototypePropertyIsEnumerable(val2, key)) {
+        }
+        const symbolKeysB = ObjectGetOwnPropertySymbols(val2);
+        if (symbolKeysA.length !== symbolKeysB.length &&
+            getEnumerables(val2, symbolKeysB).length !== count) {
+          return false;
+        }
+      } else {
+        const symbolKeysB = ObjectGetOwnPropertySymbols(val2);
+        if (symbolKeysB.length !== 0 &&
+            getEnumerables(val2, symbolKeysB).length !== 0) {
           return false;
         }
-      }
-      const symbolKeysB = ObjectGetOwnPropertySymbols(val2);
-      if (symbolKeysA.length !== symbolKeysB.length &&
-          getEnumerables(val2, symbolKeysB).length !== count) {
-        return false;
-      }
-    } else {
-      const symbolKeysB = ObjectGetOwnPropertySymbols(val2);
-      if (symbolKeysB.length !== 0 &&
-          getEnumerables(val2, symbolKeysB).length !== 0) {
-        return false;
       }
     }
   }
@@ -329,45 +329,73 @@ function keyCheck(val1, val2, strict, memos, iterationType, aKeys) {
   // Use memos to handle cycles.
   if (memos === undefined) {
     memos = {
-      val1: new SafeMap(),
-      val2: new SafeMap(),
-      position: 0
+      set: undefined,
+      a: val1,
+      b: val2,
+      c: undefined,
+      d: undefined,
+      deep: false,
+      deleteFailures: false,
     };
-  } else {
-    // We prevent up to two map.has(x) calls by directly retrieving the value
-    // and checking for undefined. The map can only contain numbers, so it is
-    // safe to check for undefined only.
-    const val2MemoA = memos.val1.get(val1);
-    if (val2MemoA !== undefined) {
-      const val2MemoB = memos.val2.get(val2);
-      if (val2MemoB !== undefined) {
-        return val2MemoA === val2MemoB;
+    return objEquiv(val1, val2, strict, aKeys, memos, iterationType);
+  }
+
+  if (memos.set === undefined) {
+    if (memos.deep === false) {
+      if (memos.a === val1) {
+        return memos.b === val2;
       }
-    }
-    memos.position++;
+      if (memos.b === val2) {
+        return false;
+      }
+      memos.c = val1;
+      memos.d = val2;
+      memos.deep = true;
+      const result = objEquiv(val1, val2, strict, aKeys, memos, iterationType);
+      memos.deep = false;
+      return result;
+    }
+    memos.set = new SafeSet();
+    memos.set.add(memos.a);
+    memos.set.add(memos.b);
+    memos.set.add(memos.c);
+    memos.set.add(memos.d);
   }
 
-  memos.val1.set(val1, memos.position);
-  memos.val2.set(val2, memos.position);
+  const { set } = memos;
+
+  const originalSize = set.size;
+  set.add(val1);
+  if (originalSize === set.size) {
+    return set.has(val2);
+  }
+  set.add(val2);
+  if (originalSize === set.size - 1) {
+    return false;
+  }
 
   const areEq = objEquiv(val1, val2, strict, aKeys, memos, iterationType);
 
-  memos.val1.delete(val1);
-  memos.val2.delete(val2);
+  if (areEq || memos.deleteFailures) {
+    set.delete(val1);
+    set.delete(val2);
+  }
 
   return areEq;
 }
 
-function setHasEqualElement(set, val1, strict, memo) {
-  // Go looking.
-  for (const val2 of set) {
+function setHasEqualElement(set, val2, strict, memo) {
+  memo.deleteFailures = true;
+  for (const val1 of set) {
     if (innerDeepEqual(val1, val2, strict, memo)) {
       // Remove the matching element to make sure we do not check that again.
-      set.delete(val2);
+      set.delete(val1);
+      memo.deleteFailures = false;
       return true;
     }
   }
 
+  memo.deleteFailures = false;
   return false;
 }
 
@@ -422,38 +450,28 @@ function setEquiv(a, b, strict, memo) {
   // pairwise.
   let set = null;
   for (const val of a) {
-    // Note: Checking for the objects first improves the performance for object
-    // heavy sets but it is a minor slow down for primitives. As they are fast
-    // to check this improves the worst case scenario instead.
-    if (typeof val === 'object' && val !== null) {
-      if (set === null) {
-        set = new SafeSet();
-      }
-      // If the specified value doesn't exist in the second set it's a non-null
-      // object (or non strict only: a not matching primitive) we'll need to go
-      // hunting for something that's 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)) {
-      if (strict)
-        return false;
-
-      // Fast path to detect missing string, symbol, undefined and null values.
-      if (!setMightHaveLoosePrim(a, b, val)) {
+    if (!b.has(val)) {
+      if ((typeof val !== 'object' || val === null) &&
+          (strict || !setMightHaveLoosePrim(a, b, val))) {
         return false;
       }
 
       if (set === null) {
+        if (b.size === 1) {
+          return innerDeepEqual(val, b.values().next().value, strict, memo);
+        }
         set = new SafeSet();
       }
+      // If the specified value doesn't exist in the second set it's a object
+      // (or in loose mode: a non-matching primitive). Find the
+      // deep-(strict-)equal element in a set copy to reduce duplicate checks.
       set.add(val);
     }
   }
 
   if (set !== null) {
     for (const val of b) {
-      // We have to check if a primitive value is already
-      // matching and only if it's not, go hunting for it.
+      // Primitive values have already been handled above.
       if (typeof val === 'object' && val !== null) {
         if (!setHasEqualElement(set, val, strict, memo))
           return false;
@@ -469,18 +487,21 @@ function setEquiv(a, b, strict, memo) {
   return true;
 }
 
-function mapHasEqualEntry(set, map, key1, item1, strict, memo) {
+function mapHasEqualEntry(set, map, key2, item2, strict, memo) {
   // To be able to handle cases like:
   //   Map([[{}, 'a'], [{}, 'b']]) vs Map([[{}, 'b'], [{}, 'a']])
   // ... we need to consider *all* matching keys, not just the first we find.
-  for (const key2 of set) {
+  memo.deleteFailures = true;
+  for (const key1 of set) {
     if (innerDeepEqual(key1, key2, strict, memo) &&
-      innerDeepEqual(item1, map.get(key2), strict, memo)) {
-      set.delete(key2);
+      innerDeepEqual(map.get(key1), item2, strict, memo)) {
+      set.delete(key1);
+      memo.deleteFailures = false;
       return true;
     }
   }
 
+  memo.deleteFailures = false;
   return false;
 }
 
@@ -490,6 +511,11 @@ function mapEquiv(a, b, strict, memo) {
   for (const { 0: key, 1: item1 } of a) {
     if (typeof key === 'object' && key !== null) {
       if (set === null) {
+        if (b.size === 1) {
+          const { 0: key2, 1: item2 } = b.entries().next().value;
+          return innerDeepEqual(key, key2, strict, memo) &&
+                 innerDeepEqual(item1, item2, strict, memo);
+        }
         set = new SafeSet();
       }
       set.add(key);
@@ -520,8 +546,8 @@ function mapEquiv(a, b, strict, memo) {
           return false;
       } else if (!strict &&
                  (!a.has(key) ||
-                   !innerDeepEqual(a.get(key), item, false, memo)) &&
-                 !mapHasEqualEntry(set, a, key, item, false, memo)) {
+                   !innerDeepEqual(a.get(key), item, strict, memo)) &&
+                 !mapHasEqualEntry(set, a, key, item, strict, memo)) {
         return false;
       }
     }
@@ -532,20 +558,17 @@ function mapEquiv(a, b, strict, memo) {
 }
 
 function objEquiv(a, b, strict, keys, memos, iterationType) {
-  // Sets and maps don't have their entries accessible via normal object
-  // properties.
-  let i = 0;
-
-  if (iterationType === kIsSet) {
-    if (!setEquiv(a, b, strict, memos)) {
-      return false;
-    }
-  } else if (iterationType === kIsMap) {
-    if (!mapEquiv(a, b, strict, memos)) {
-      return false;
+  // The pair must have equivalent values for every corresponding key.
+  if (keys.length > 0) {
+    for (const key of keys) {
+      if (!innerDeepEqual(a[key], b[key], strict, memos)) {
+        return false;
+      }
     }
-  } else if (iterationType === kIsArray) {
-    for (; i < a.length; i++) {
+  }
+
+  if (iterationType === kIsArray) {
+    for (let i = 0; i < a.length; i++) {
       if (ObjectPrototypeHasOwnProperty(a, i)) {
         if (!ObjectPrototypeHasOwnProperty(b, i) ||
             !innerDeepEqual(a[i], b[i], strict, memos)) {
@@ -563,22 +586,19 @@ function objEquiv(a, b, strict, keys, memos, iterationType) {
             return false;
           }
         }
-        if (keysA.length !== ObjectKeys(b).length) {
-          return false;
-        }
-        return true;
+        return keysA.length === ObjectKeys(b).length;
       }
     }
-  }
-
-  // The pair must have equivalent values for every corresponding key.
-  // Possibly expensive deep test:
-  for (i = 0; i < keys.length; i++) {
-    const key = keys[i];
-    if (!innerDeepEqual(a[key], b[key], strict, memos)) {
+  } else if (iterationType === kIsSet) {
+    if (!setEquiv(a, b, strict, memos)) {
+      return false;
+    }
+  } else if (iterationType === kIsMap) {
+    if (!mapEquiv(a, b, strict, memos)) {
       return false;
     }
   }
+
   return true;
 }
 
diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js
index 013c7985f51d2a..d4334b584549b9 100644
--- a/test/parallel/test-assert-deep.js
+++ b/test/parallel/test-assert-deep.js
@@ -419,21 +419,33 @@ assertNotDeepOrStrict(
 // GH-14441. Circular structures should be consistent
 {
   const a = {};
-  const b = {};
   a.a = a;
+
+  const b = {};
   b.a = {};
   b.a.a = a;
-  assertDeepAndStrictEqual(a, b);
+
+  assertNotDeepOrStrict(a, b);
+}
+
+{
+  const a = {};
+  a.a = a;
+  const b = {};
+  b.a = b;
+  const c = {};
+  c.a = a;
+  assertNotDeepOrStrict(b, c);
 }
 
 {
   const a = new Set();
-  const b = new Set();
-  const c = new Set();
   a.add(a);
+  const b = new Set();
   b.add(b);
+  const c = new Set();
   c.add(a);
-  assertDeepAndStrictEqual(b, c);
+  assertNotDeepOrStrict(b, c);
 }
 
 // https://github.com/nodejs/node-v0.x-archive/pull/7178