perf(copy): only validate/clear user specified destination

angular · jbedard · Jun 9, 2015 · Sep 22, 2015 · d0a665d2539ee28db772ed474f5c77dc99a0033f
commit d0a665d2539ee28db772ed474f5c77dc99a0033f
diff --git a/src/Angular.js b/src/Angular.js
@@ -794,102 +794,108 @@ function arrayRemove(array, value) {
  </file>
  </example>
  */
-function copy(source, destination, stackSource, stackDest) {
-  if (isWindow(source) || isScope(source)) {
-    throw ngMinErr('cpws',
-      "Can't copy! Making copies of Window or Scope instances is not supported.");
-  }
-  if (isTypedArray(destination)) {
-    throw ngMinErr('cpta',
-      "Can't copy! TypedArray destination cannot be mutated.");
-  }
-
-  if (!destination) {
-    destination = source;
-    if (isObject(source)) {
-      var index;
-      if (stackSource && (index = stackSource.indexOf(source)) !== -1) {
-        return stackDest[index];
-      }
-
-      // TypedArray, Date and RegExp have specific copy functionality and must be
-      // pushed onto the stack before returning.
-      // Array and other objects create the base object and recurse to copy child
-      // objects. The array/object will be pushed onto the stack when recursed.
-      if (isArray(source)) {
-        return copy(source, [], stackSource, stackDest);
-      } else if (isTypedArray(source)) {
-        destination = new source.constructor(source);
-      } else if (isDate(source)) {
-        destination = new Date(source.getTime());
-      } else if (isRegExp(source)) {
-        destination = new RegExp(source.source, source.toString().match(/[^\/]*$/)[0]);
-        destination.lastIndex = source.lastIndex;
-      } else if (isFunction(source.cloneNode)) {
-          destination = source.cloneNode(true);
-      } else {
-        var emptyObject = Object.create(getPrototypeOf(source));
-        return copy(source, emptyObject, stackSource, stackDest);
-      }
-
-      if (stackDest) {
-        stackSource.push(source);
-        stackDest.push(destination);
-      }
+function copy(source, destination) {
+  if (destination) {
+    if (isTypedArray(destination)) {
+      throw ngMinErr('cpta', "Can't copy! TypedArray destination cannot be mutated.");
     }
-  } else {
-    if (source === destination) throw ngMinErr('cpi',
-      "Can't copy! Source and destination are identical.");
-
-    stackSource = stackSource || [];
-    stackDest = stackDest || [];
-
-    if (isObject(source)) {
-      stackSource.push(source);
-      stackDest.push(destination);
+    if (source === destination) {
+      throw ngMinErr('cpi', "Can't copy! Source and destination are identical.");
     }
 
-    var result, key;
-    if (isArray(source)) {
+    // Empty the destination object
+    if (isArray(destination)) {
       destination.length = 0;
-      for (var i = 0; i < source.length; i++) {
-        destination.push(copy(source[i], null, stackSource, stackDest));
-      }
     } else {
-      var h = destination.$$hashKey;
-      if (isArray(destination)) {
-        destination.length = 0;
-      } else {
-        forEach(destination, function(value, key) {
+      forEach(destination, function(value, key) {
+        if (key !== '$$hashKey') {
           delete destination[key];
-        });
-      }
-      if (isBlankObject(source)) {
-        // createMap() fast path --- Safe to avoid hasOwnProperty check because prototype chain is empty
-        for (key in source) {
-          destination[key] = copy(source[key], null, stackSource, stackDest);
-        }
-      } else if (source && typeof source.hasOwnProperty === 'function') {
-        // Slow path, which must rely on hasOwnProperty
-        for (key in source) {
-          if (source.hasOwnProperty(key)) {
-            destination[key] = copy(source[key], null, stackSource, stackDest);
-          }
-        }
-      } else {
-        // Slowest path --- hasOwnProperty can't be called as a method
-        for (key in source) {
-          if (hasOwnProperty.call(source, key)) {
-            destination[key] = copy(source[key], null, stackSource, stackDest);
-          }
         }
+      });
+    }
+
+    return _copyRecurse(source, destination, [source], [destination]);
+  }
+
+  return _copy(source, [], []);
+}
+
+function _copyRecurse(source, destination, stackSource, stackDest) {
+  var h = destination.$$hashKey;
+  var result, key;
+  if (isArray(source)) {
+    for (var i = 0, ii = source.length; i < ii; i++) {
+      destination.push(_copy(source[i], stackSource, stackDest));
+    }
+  } else if (isBlankObject(source)) {
+    // createMap() fast path --- Safe to avoid hasOwnProperty check because prototype chain is empty
+    for (key in source) {
+      destination[key] = _copy(source[key], stackSource, stackDest);
+    }
+  } else if (source && typeof source.hasOwnProperty === 'function') {
+    // Slow path, which must rely on hasOwnProperty
+    for (key in source) {
+      if (source.hasOwnProperty(key)) {
+        destination[key] = _copy(source[key], stackSource, stackDest);
+      }
+    }
+  } else {
+    // Slowest path --- hasOwnProperty can't be called as a method
+    for (key in source) {
+      if (hasOwnProperty.call(source, key)) {
+        destination[key] = _copy(source[key], stackSource, stackDest);
       }
-      setHashKey(destination,h);
     }
   }
+  setHashKey(destination, h);
   return destination;
 }
 
+function _copy(source, stackSource, stackDest) {
+  // Simple values
+  if (!isObject(source)) {
+    return source;
+  }
+
+  // Already copied values
+  var index = stackSource.indexOf(source);
+  if (index !== -1) {
+    return stackDest[index];
+  }
+
+  if (isWindow(source) || isScope(source)) {
+    throw ngMinErr('cpws',
+      "Can't copy! Making copies of Window or Scope instances is not supported.");
+  }
+
+  var needsRecurse = false;
+  var destination;
+
+  if (isArray(source)) {
+    destination = [];
+    needsRecurse = true;
+  } else if (isTypedArray(source)) {
+    destination = new source.constructor(source);
+  } else if (isDate(source)) {
+    destination = new Date(source.getTime());
+  } else if (isRegExp(source)) {
+    destination = new RegExp(source.source, source.toString().match(/[^\/]*$/)[0]);
+    destination.lastIndex = source.lastIndex;
+  } else if (isFunction(source.cloneNode)) {
+      destination = source.cloneNode(true);
+  } else {
+    destination = Object.create(getPrototypeOf(source));
+    needsRecurse = true;
+  }
+
+  stackSource.push(source);
+  stackDest.push(destination);
+
+  return needsRecurse
+    ? _copyRecurse(source, destination, stackSource, stackDest)
+    : destination;
+}
+
 /**
  * Creates a shallow copy of an object, an array or a primitive.
  *

diff --git a/test/AngularSpec.js b/test/AngularSpec.js
@@ -313,11 +313,19 @@ describe('angular', function() {
     it('should throw an exception if a Scope is being copied', inject(function($rootScope) {
       expect(function() { copy($rootScope.$new()); }).
           toThrowMinErr("ng", "cpws", "Can't copy! Making copies of Window or Scope instances is not supported.");
+      expect(function() { copy({child: $rootScope.$new()}, {}); }).
+          toThrowMinErr("ng", "cpws", "Can't copy! Making copies of Window or Scope instances is not supported.");
+      expect(function() { copy([$rootScope.$new()]); }).
+          toThrowMinErr("ng", "cpws", "Can't copy! Making copies of Window or Scope instances is not supported.");
     }));
 
     it('should throw an exception if a Window is being copied', function() {
       expect(function() { copy(window); }).
           toThrowMinErr("ng", "cpws", "Can't copy! Making copies of Window or Scope instances is not supported.");
+      expect(function() { copy({child: window}); }).
+          toThrowMinErr("ng", "cpws", "Can't copy! Making copies of Window or Scope instances is not supported.");
+      expect(function() { copy([window], []); }).
+          toThrowMinErr("ng", "cpws", "Can't copy! Making copies of Window or Scope instances is not supported.");
     });
 
     it('should throw an exception when source and destination are equivalent', function() {
@@ -334,6 +342,11 @@ describe('angular', function() {
       hashKey(src);
       dst = copy(src);
       expect(hashKey(dst)).not.toEqual(hashKey(src));
+
+      src = {foo: {}};
+      hashKey(src.foo);
+      dst = copy(src);
+      expect(hashKey(src.foo)).not.toEqual(hashKey(dst.foo));
     });
 
     it('should retain the previous $$hashKey when copying object with hashKey', function() {