address preformance regression introduced in pr 845 #852

Merged
merged 4 commits into from Dec 11, 2012

2 participants

@drewfish
Yahoo Inc. member

No description provided.

@caridy caridy and 1 other commented on an outdated diff Dec 11, 2012
lib/app/autoload/store.server.js
@@ -722,7 +725,7 @@ YUI.add('mojito-resource-store', function(Y, NAME) {
Y.log('The "dest" parameter to store.getMojitTypeDetails() is deprecated.', 'warn', NAME);
Y.mojito.util.mergeRecursive(dest, cacheValue);
}
- return Y.mojito.util.copy(cacheValue);
+ return cacheValue;
@caridy
caridy added a note Dec 11, 2012

This is pretty risky because getMojitTypeDetails is a public method, and people will use it in many different ways, they might end up corrupting the cached data. Can we pass a flag or something if we don't want to make a copy intentionally to make it work more performant?

@drewfish
Yahoo Inc. member

Yes I agree.

However, since in expandInstanceForEnv() we're using the results as part of a blend3() the extra copy doesn't do anything, and removing it lead to a very noticeable performance gain.

I'll add the flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mojit0 mojit0 and 1 other commented on an outdated diff Dec 11, 2012
lib/app/autoload/util.common.js
}
+ return overlay.slice(0);
@mojit0
mojit0 added a note Dec 11, 2012

I think I'd put a warn: here so people are aware we're throwing away the base object and just copying their input.

@mojit0
mojit0 added a note Dec 11, 2012

Note that this alters the semantics of the prior version, which let dest fall through to the final loop which would copy over properties. As long as we only want the blending operation to work on "content" this version is fine. But if we were to put any metadata on the Array instance it would be lost in this new version.

@drewfish
Yahoo Inc. member

OK. Presumably that's their intent, if they're replacing a string with an array then they intend for the array to be used.

@drewfish
Yahoo Inc. member

I think blend() and blend3() (and copy() and mergeRecursive()) are intended to work on JSON-like objects/arrays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mojit0 mojit0 commented on the diff Dec 11, 2012
tests/base/mojito-test.js
Y.MOJITO_DIR = require ?
require('path').resolve(__dirname, '../../') + '/' : null;
-});
+
+ // path doesn't need to be given, mainly used during recursion
+ Y.TEST_CMP = function (x, y, msg, path) {
+ path = path || 'obj';
+ var i;
+ if (Y.Lang.isArray(x)) {
+ A.isArray(x, path + ': ' + (msg || 'first arg should be an array'));
+ A.isArray(y, path + ': ' + (msg || 'second arg should be an array'));
+ A.areSame(x.length, y.length, path + ': ' + (msg || 'arrays are different lengths'));
+ for (i = 0; i < x.length; i += 1) {
@mojit0
mojit0 added a note Dec 11, 2012

Any reason you're not using ArrayAssert here?

@drewfish
Yahoo Inc. member

I copied this code from another file, so I'm not sure why. I think it's because ArrayAssert doesn't do a recursive comparison of values if the array contains things like objects or other arrays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mojit0 mojit0 commented on the diff Dec 11, 2012
lib/app/autoload/util.common.js
- // Verify we didn't miss properties in the base. Anything
- // missing from the destination object should be copied.
- // Anything already there would have been merged in the previous
- // loop.
+ // Verify we didn't miss properties in the base. Anything
+ // missing from the destination object should be copied.
+ // Anything already there would have been merged in the previous
+ // loop.
+ if (Y.Lang.isObject(base) && !Y.Lang.isArray(base)) {
for (i in base) {
@mojit0
mojit0 added a note Dec 11, 2012

See earlier note about metadata on arrays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mojit0 mojit0 commented on an outdated diff Dec 11, 2012
lib/app/autoload/util.common.js
+
+ // Otherwise treat everything as an object.
+ dest = {};
+ if ((!Y.Lang.isObject(medium)) || Y.Lang.isArray(medium)) {
+ medium = null;
+ lowest = null;
+ }
+ if ((!Y.Lang.isObject(lowest)) || Y.Lang.isArray(lowest)) {
+ lowest = null;
+ }
+ for (key in highest) {
+ if (highest.hasOwnProperty(key)) {
+ val = highest[key];
+ if (Y.Lang.isObject(val)) {
+ if (medium && medium.hasOwnProperty(key)) {
+ if (lowest && lowest.hasOwnProperty(key)) {
@mojit0
mojit0 added a note Dec 11, 2012

Might be able to squeeze a bit more out of this by not checking medium and lowest repeatedly. I think these checks are going to do implicit Boolean conversions with each iteration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mojit0

+1

@drewfish drewfish merged commit 9d6ceea into yahoo:develop Dec 11, 2012

1 check was pending

Details default The Travis build is in progress
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment