Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

perf(copy): use ES6 Map to track source/destination objects #13209

Closed
wants to merge 4 commits into from
Closed

perf(copy): use ES6 Map to track source/destination objects #13209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d8f593c
perf(copy): use ES6 Map to track source/destination objects
jbedard Oct 30, 2015
ed16ce1
fixup! perf(copy): Saving the last key/index lookup in ESMapShim
jbedard Dec 11, 2015
6a102ef
fixup! perf(copy): replace Map feature-test with native-test
jbedard Aug 27, 2016
5606fe8
refactor(copy): avoid creating source/dest Map for simple copies
jbedard Aug 28, 2016
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
66 changes: 50 additions & 16 deletions src/Angular.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,46 @@ function arrayRemove(array, value) {
return index;
}

// A minimal ES6 Map implementation.
// Should be bug/feature equivelent to the native implementations of supported browsers.
Copy link
Member

@gkalpak gkalpak Sep 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

equivelent --> equivalent

// See https://kangax.github.io/compat-table/es6/#test-Map
function ES6MapShim() {
this._keys = [];
this._values = [];
this._lastKey = NaN;
this._lastIndex = -1;
}
ES6MapShim.prototype = {
_idx: function(key) {
if (key === this._lastKey) {
return this._lastIndex;
}
return (this._lastIndex = (this._keys.indexOf(this._lastKey = key)));
},
get: function(key) {
var idx = this._idx(key);
if (idx !== -1) {
return this._values[idx];
}
},
set: function(key, value) {
var idx = this._idx(key);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we abuse of the fact that we know we are never going to override a value?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am thinking something like

set: function(key, value) {
  this._keys.push(key);
  this._values.push(value);
}
get: function(key) {
  var idx = this._keys.lastIndexOf(key);
  if (idx != -1) return this._values[idx];
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could for this copy specific use case. But I was trying to keep this shim equivalent to the native ES6 version so it could be used anywhere...

if (idx === -1) {
idx = this._lastIndex = this._keys.length;
this._lastKey = key;
}
this._keys[idx] = key;
this._values[idx] = value;

// Support: IE11
// Do not `return this` to simulate the partial IE11 implementation
Copy link
Member

Choose a reason for hiding this comment

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

LOL

}
};

var ES6Map = isFunction(window.Map) && toString.call(window.Map.prototype) === '[object Map]'
? window.Map
: ES6MapShim;

/**
* @ngdoc function
* @name angular.copy
Expand Down Expand Up @@ -809,8 +849,7 @@ function arrayRemove(array, value) {
</example>
*/
function copy(source, destination) {
var stackSource = [];
var stackDest = [];
var stack;

if (destination) {
if (isTypedArray(destination) || isArrayBuffer(destination)) {
Expand All @@ -831,14 +870,14 @@ function copy(source, destination) {
});
}

stackSource.push(source);
stackDest.push(destination);
return copyRecurse(source, destination);
}

return copyElement(source);

function copyRecurse(source, destination) {
(stack || (stack = new ES6Map())).set(source, destination);

var h = destination.$$hashKey;
var key;
if (isArray(source)) {
Expand Down Expand Up @@ -876,30 +915,25 @@ function copy(source, destination) {
}

// Already copied values
var index = stackSource.indexOf(source);
if (index !== -1) {
return stackDest[index];
var existingCopy = stack && stack.get(source);
if (existingCopy) {
return existingCopy;
}

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 = copyType(source);

if (destination === undefined) {
destination = isArray(source) ? [] : Object.create(getPrototypeOf(source));
needsRecurse = true;
destination = copyRecurse(source, isArray(source) ? [] : Object.create(getPrototypeOf(source)));
} else if (stack) {
stack.set(source, destination);
}

stackSource.push(source);
stackDest.push(destination);

return needsRecurse
? copyRecurse(source, destination)
: destination;
return destination;
}

function copyType(source) {
Expand Down
2 changes: 2 additions & 0 deletions test/.eslintrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@
"getBlockNodes": false,
"createMap": false,
"VALIDITY_STATE_PROPERTY": true,
"testES6Map": true,
Copy link
Member

Choose a reason for hiding this comment

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

Redundant

"ES6MapShim": true,
Copy link
Member

Choose a reason for hiding this comment

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

Isn't that redundant too, actually?


/* AngularPublic.js */
"version": false,
Expand Down