util,assert: improve performance

This significantly improves regular and typed array performance by not checking the indices keys anymore. This can be done with a V8 API that allows to only retrieve the non indices property keys. PR-URL: #22197 Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
nodejs · Sep 3, 2018 · 2b1cb3b · 2b1cb3b
1 parent c17e980
commit 2b1cb3b
Show file tree

Hide file tree

Showing 3 changed files with 73 additions and 44 deletions.
diff --git a/lib/internal/util/comparisons.js b/lib/internal/util/comparisons.js
@@ -4,6 +4,7 @@ const { compare } = process.binding('buffer');
 const { isArrayBufferView } = require('internal/util/types');
 const { internalBinding } = require('internal/bootstrap/loaders');
 const { isDate, isMap, isRegExp, isSet } = internalBinding('types');
+const { getOwnNonIndexProperties } = process.binding('util');
 
 const ReflectApply = Reflect.apply;
 
@@ -106,24 +107,11 @@ function strictDeepEqual(val1, val2, memos) {
     if (val1.length !== val2.length) {
       return false;
     }
-    const keys = objectKeys(val1);
-    if (keys.length !== objectKeys(val2).length) {
+    const keys1 = getOwnNonIndexProperties(val1);
+    if (keys1.length !== getOwnNonIndexProperties(val2).length) {
       return false;
     }
-    // Fast path for non sparse arrays (no key comparison for indices
-    // properties).
-    // See https://tc39.github.io/ecma262/#sec-ordinaryownpropertykeys
-    if (val1.length === keys.length) {
-      if (keys.length === 0 || keys[val1.length - 1] === `${val1.length - 1}`) {
-        return keyCheck(val1, val2, kStrict, memos, kIsArray, []);
-      }
-    } else if (keys.length > val1.length &&
-               keys[val1.length - 1] === `${val1.length - 1}`) {
-      const minimalKeys = keys.slice(val1.length);
-      return keyCheck(val1, val2, kStrict, memos, kIsArray, minimalKeys);
-    }
-    // Only set this to kIsArray in case the array is not sparse!
-    return keyCheck(val1, val2, kStrict, memos, kNoIterator, keys);
+    return keyCheck(val1, val2, kStrict, memos, kIsArray, keys1);
   }
   if (val1Tag === '[object Object]') {
     return keyCheck(val1, val2, kStrict, memos, kNoIterator);
@@ -148,18 +136,14 @@ function strictDeepEqual(val1, val2, memos) {
     if (!areSimilarTypedArrays(val1, val2)) {
       return false;
     }
-    // Buffer.compare returns true, so val1.length === val2.length
-    // if they both only contain numeric keys, we don't need to exam further.
-    const keys = objectKeys(val1);
-    if (keys.length !== objectKeys(val2).length) {
+    // Buffer.compare returns true, so val1.length === val2.length. If they both
+    // only contain numeric keys, we don't need to exam further than checking
+    // the symbols.
+    const keys1 = getOwnNonIndexProperties(val1);
+    if (keys1.length !== getOwnNonIndexProperties(val2).length) {
       return false;
     }
-    if (keys.length === val1.length) {
-      return keyCheck(val1, val2, kStrict, memos, kNoIterator, []);
-    }
-    // Only compare the special keys.
-    const minimalKeys = keys.slice(val1.length);
-    return keyCheck(val1, val2, kStrict, memos, kNoIterator, minimalKeys);
+    return keyCheck(val1, val2, kStrict, memos, kNoIterator, keys1);
   } else if (isSet(val1)) {
     if (!isSet(val2) || val1.size !== val2.size) {
       return false;
@@ -173,21 +157,10 @@ function strictDeepEqual(val1, val2, memos) {
   // TODO: Make the valueOf checks safe.
   } else if (typeof val1.valueOf === 'function') {
     const val1Value = val1.valueOf();
-    if (val1Value !== val1) {
-      if (typeof val2.valueOf !== 'function') {
-        return false;
-      }
-      if (!innerDeepEqual(val1Value, val2.valueOf(), true))
-        return false;
-      // Fast path for boxed primitive strings.
-      if (typeof val1Value === 'string') {
-        const keys = objectKeys(val1);
-        if (keys.length !== objectKeys(val2).length) {
-          return false;
-        }
-        const minimalKeys = keys.slice(val1.length);
-        return keyCheck(val1, val2, kStrict, memos, kNoIterator, minimalKeys);
-      }
+    if (val1Value !== val1 &&
+        (typeof val2.valueOf !== 'function' ||
+          !innerDeepEqual(val1Value, val2.valueOf(), kStrict))) {
+      return false;
     }
   }
   return keyCheck(val1, val2, kStrict, memos, kNoIterator);
@@ -277,7 +250,7 @@ function keyCheck(val1, val2, strict, memos, iterationType, aKeys) {
     }
   }
 
-  if (strict) {
+  if (strict && arguments.length === 5) {
     const symbolKeysA = getOwnPropertySymbols(val1);
     if (symbolKeysA.length !== 0) {
       let count = 0;
@@ -310,7 +283,7 @@ function keyCheck(val1, val2, strict, memos, iterationType, aKeys) {
   if (aKeys.length === 0 &&
       (iterationType === kNoIterator ||
         iterationType === kIsArray && val1.length === 0 ||
-      val1.size === 0)) {
+        val1.size === 0)) {
     return true;
   }
 
@@ -578,8 +551,28 @@ function objEquiv(a, b, strict, keys, memos, iterationType) {
     }
   } else if (iterationType === kIsArray) {
     for (; i < a.length; i++) {
-      if (!innerDeepEqual(a[i], b[i], strict, memos)) {
+      if (hasOwnProperty(a, i)) {
+        if (!hasOwnProperty(b, i) ||
+            !innerDeepEqual(a[i], b[i], strict, memos)) {
+          return false;
+        }
+      } else if (hasOwnProperty(b, i)) {
         return false;
+      } else {
+        // Array is sparse.
+        const keysA = objectKeys(a);
+        i++;
+        for (; i < keysA.length; i++) {
+          const key = keysA[i];
+          if (!hasOwnProperty(b, key) ||
+              !innerDeepEqual(a[key], b[i], strict, memos)) {
+            return false;
+          }
+        }
+        if (keysA.length !== objectKeys(b).length) {
+          return false;
+        }
+        return true;
       }
     }
   }

diff --git a/src/node_util.cc b/src/node_util.cc
@@ -16,6 +16,29 @@ using v8::Proxy;
 using v8::String;
 using v8::Value;
 
+static void GetOwnNonIndexProperties(
+    const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+  Local<Context> context = env->context();
+
+  if (!args[0]->IsObject())
+    return;
+
+  v8::Local<v8::Object> object = args[0].As<v8::Object>();
+
+  // Return only non-enumerable properties by default.
+  v8::Local<v8::Array> properties;
+
+  if (!object->GetPropertyNames(
+        context, v8::KeyCollectionMode::kOwnOnly,
+        v8::ONLY_ENUMERABLE,
+        v8::IndexFilter::kSkipIndices)
+          .ToLocal(&properties)) {
+    return;
+  }
+  args.GetReturnValue().Set(properties);
+}
+
 static void GetPromiseDetails(const FunctionCallbackInfo<Value>& args) {
   // Return undefined if it's not a Promise.
   if (!args[0]->IsPromise())
@@ -177,6 +200,8 @@ void Initialize(Local<Object> target,
   env->SetMethodNoSideEffect(target, "getProxyDetails", GetProxyDetails);
   env->SetMethodNoSideEffect(target, "safeToString", SafeToString);
   env->SetMethodNoSideEffect(target, "previewEntries", PreviewEntries);
+  env->SetMethodNoSideEffect(target, "getOwnNonIndexProperties",
+                                     GetOwnNonIndexProperties);
 
   env->SetMethod(target, "startSigintWatchdog", StartSigintWatchdog);
   env->SetMethod(target, "stopSigintWatchdog", StopSigintWatchdog);

diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js
@@ -934,6 +934,17 @@ assert.throws(() => assert.deepStrictEqual(new Boolean(true), {}),
                '  [\n    1,\n-   2\n+   2,\n+   3\n  ]' }
   );
   util.inspect.defaultOptions = tmp;
+
+  const invalidTrap = new Proxy([1, 2, 3], {
+    ownKeys() { return []; }
+  });
+  assert.throws(
+    () => assert.deepStrictEqual(invalidTrap, [1, 2, 3]),
+    {
+      name: 'TypeError',
+      message: "'ownKeys' on proxy: trap result did not include 'length'"
+    }
+  );
 }
 
 // Basic valueOf check.