Skip to content
This repository has been archived by the owner on Oct 30, 2018. It is now read-only.

address preformance regression introduced in pr 845 #852

Merged
merged 4 commits into from
Dec 11, 2012
Merged

address preformance regression introduced in pr 845 #852

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
836189d
address preformance regression introduced in pr 845
drewfish Dec 11, 2012
ff9677a
feedback from code review
drewfish Dec 11, 2012
bcb6516
delinted
drewfish Dec 11, 2012
6081cc2
more feedback from code review
drewfish Dec 11, 2012
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
46 changes: 11 additions & 35 deletions lib/app/autoload/store.server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,8 +574,7 @@ YUI.add('mojito-resource-store', function(Y, NAME) {
// * instance
// * spec
// * typeDetails
newInst = Y.mojito.util.blend(typeDetails, spec);
newInst = Y.mojito.util.mergeRecursive(newInst, instance);
newInst = Y.mojito.util.blend3(typeDetails, spec, instance);
cb(null, newInst);
},

Expand All @@ -586,6 +585,10 @@ YUI.add('mojito-resource-store', function(Y, NAME) {
* As the last step of execution, this fires the `getMojitTypeDetails`
* event so that Resource Store addons can augment the returned structure.
*
* NOTE! This returns an object which is shared with similar calls to
* this method. If you intend to modify the object please make a deep
* copy first and use that instead.
*
* @method getMojitTypeDetails
* @param {string} env the runtime environment (either `client` or `server`)
* @param {object} ctx the context
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

},


Expand Down Expand Up @@ -1563,37 +1566,10 @@ YUI.add('mojito-resource-store', function(Y, NAME) {
*/
// FUTURE: expose this to RS addons?
_expandSpec: function(env, ctx, spec) {
// [issue 835] The context is part of the cache key
// since the spec can vary in the application json.
var cacheKey = JSON.stringify([env, ctx, spec]),
cacheValue = this._expandSpecCache[cacheKey];

// We do the caching in this wrapper because the expandSpec
// algorithm is recursive and we don't want to keep doing
// the cache checks at each iteration.
// We -could- do caching within _expandSpecRecursive() for
// better cache re-use of expanded bases, but it's not
// (currently) expected that apps will have lots (1000s)
// of specs.
if (!cacheValue) {
cacheValue = this._expandSpecRecursive(env, ctx, spec);
this._expandSpecCache[cacheKey] = cacheValue;
}

return Y.mojito.util.copy(cacheValue);
},


/**
* Recursively build the spec by following base.
* @private
* @method _expandSpecRecursive
* @param {string} env the runtime environment (either `client` or `server`)
* @param {object} ctx runtime context
* @param {object} spec spec to expand
* @return {object} expanded sped
*/
_expandSpecRecursive: function(env, ctx, spec) {
// We could add caching in here, but it turns out that it's faster
// not to. This algorithm is pretty simple and a lot of the heavy
// lifting is being done inside the YCB library which has its own
// caching.
var appConfig,
base,
specParts,
Expand Down Expand Up @@ -1628,7 +1604,7 @@ YUI.add('mojito-resource-store', function(Y, NAME) {

spec.base = undefined;
return Y.mojito.util.mergeRecursive(
this._expandSpecRecursive(env, ctx, base),
this._expandSpec(env, ctx, base),
spec
);
},
Expand Down
146 changes: 127 additions & 19 deletions lib/app/autoload/util.common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,8 @@ YUI.add('mojito-util', function(Y) {
* object containing the recursively merged values of the two. This is
* similar to performing a copy and then a mergeRecursive operation but
* is intended to be more performant by avoiding duplicate work.
*
* @method blend
* @param {Object} base The base object. Properties in this object may
* be overwritten by those in overlay.
* @param {Object} extension The "overlay" object. Properties in this
Expand All @@ -240,11 +242,9 @@ YUI.add('mojito-util', function(Y) {
*/
blend: function(base, overlay) {

var tmp,
dest,
var dest,
i,
slot,
slotType,
copy = Y.mojito.util.copy,
blend = Y.mojito.util.blend;

Expand All @@ -257,14 +257,11 @@ YUI.add('mojito-util', function(Y) {
}

if (Y.Lang.isArray(overlay)) {
if (!Y.Lang.isArray(base)) {
throw new Error('Type mismatch for object merge.');
if (Y.Lang.isArray(base)) {
return Y.Array.unique(base.concat(overlay));
} else {
return overlay.slice(0);
}

tmp = base.slice(0);
tmp.push.apply(tmp, overlay);
dest = Y.Array.unique(tmp);
tmp.length = 0;
} else {
dest = {};
for (i in overlay) {
Expand All @@ -273,14 +270,12 @@ YUI.add('mojito-util', function(Y) {
// "Conflicting" property. A recursive merge of the
// value is required if it's a complex object.
slot = overlay[i];
slotType = typeof slot;
if ((slotType === 'object') &&
(slot !== null)) {
if (Y.Lang.isObject(slot)) {
// Complex, not null.
dest[i] = blend(base[i], overlay[i]);
dest[i] = blend(base[i], slot);
} else {
// Simple value or null. Overlay value wins.
dest[i] = overlay[i];
dest[i] = slot;
}
} else {
// Unique to the overlay object, no merge needed.
Expand All @@ -293,14 +288,127 @@ YUI.add('mojito-util', function(Y) {
// missing from the destination object should be copied.
// Anything already there would have been merged in the previous
// loop.
for (i in base) {
if (base.hasOwnProperty(i)) {
if (!dest.hasOwnProperty(i)) {
dest[i] = copy(base[i]);
if (Y.Lang.isObject(base) && !Y.Lang.isArray(base)) {
for (i in base) {
if (base.hasOwnProperty(i)) {
if (!dest.hasOwnProperty(i)) {
dest[i] = copy(base[i]);
}
}
}
}
}
return dest;
},


/**
* Blends three objects to create a new object containing the
* recursively merged values of the them all. This is similar to
* performing copy()s and then mergeRecursive()s but is intended
* to be more performant by avoiding duplicate work.
*
* The type of the "highest" parameter always dictates type of results.
*
* @method blend3
* @param {Object} lowest The lowest priority object.
* Properties in this object may be overwritten by those in medium and highest.
* @param {Object} medium The medium priority object.
* Properties in this object may be overwritten by those in highest.
* @param {Object} highest The highest priority object.
* Properties in this object are always found in the results.
* @return {Object} A new object containing the blended values.
*/
blend3: function(lowest, medium, highest) {
var dest,
key,
val,
copy = Y.mojito.util.copy,
blend2 = Y.mojito.util.blend,
blend3 = Y.mojito.util.blend3;

// Protect from bad input data.
if (highest === null || highest === undefined) {
return blend2(lowest, medium);
}

if (Y.Lang.isArray(highest)) {
if (Y.Lang.isArray(medium)) {
if (Y.Lang.isArray(lowest)) {
dest = lowest.concat(medium, highest);
} else {
dest = medium.concat(highest);
}
} else {
dest = highest.slice(0);
}
return Y.Array.unique(dest);
}

// 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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

dest[key] = blend3(lowest[key], medium[key], val);
} else {
dest[key] = blend2(medium[key], val);
}
} else {
if (lowest && lowest.hasOwnProperty(key)) {
dest[key] = blend2(lowest[key], val);
} else {
dest[key] = copy(val);
}
}
} else {
dest[key] = copy(val);
}
}
}

// Process keys that are in medium but not in highest.
if (medium) {
for (key in medium) {
if (medium.hasOwnProperty(key)) {
if (!dest.hasOwnProperty(key)) {
val = medium[key];
if (Y.Lang.isObject(val)) {
if (lowest && lowest.hasOwnProperty(key)) {
dest[key] = blend2(lowest[key], val);
} else {
dest[key] = copy(val);
}
} else {
// scalar
dest[key] = val;
}
}
}
}
}

// Process keys that are in lowest but not highest nor in medium.
if (lowest) {
for (key in lowest) {
if (lowest.hasOwnProperty(key)) {
if (!dest.hasOwnProperty(key)) {
dest[key] = copy(lowest[key]);
}
}
}
}

return dest;
},

Expand Down
32 changes: 31 additions & 1 deletion tests/base/mojito-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,36 @@ YUI.add('mojito-test', function(Y, NAME) {


YUI.add('mojito-test-extra', function(Y, NAME) {
var A = Y.Assert;

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason you're not using ArrayAssert here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Y.TEST_CMP(x[i], y[i], msg, path + '[' + i + ']');
}
return;
}
if (Y.Lang.isObject(x)) {
A.isObject(x, path + ': ' + (msg || 'first arg should be an object'));
A.isObject(y, path + ': ' + (msg || 'second arg should be an object'));
A.areSame(Object.keys(x).length, Object.keys(y).length, path + ': ' + (msg || 'object keys are different lengths'));
for (i in x) {
if (x.hasOwnProperty(i)) {
Y.TEST_CMP(x[i], y[i], msg, path + '{' + i + '}');
}
}
return;
}
A.areSame(x, y, path + ': ' + (msg || 'args should be the same'));
};

}, '0.1.0', {requires: ['test']});
15 changes: 0 additions & 15 deletions tests/unit/lib/app/autoload/test-store.server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,21 +261,6 @@ YUI().use(
});
},

'expandSpec caching': function() {
var inSpec = {
base: 'a',
type: 'c'
};
var env = 'server';
var ctx = {};
var key = Y.JSON.stringify([env, ctx, inSpec]);
store._expandSpecCache[key] = { x: 'y' };
var outSpec = store._expandSpec(env, ctx, inSpec);
A.isObject(outSpec);
A.areEqual(1, Object.keys(outSpec).length);
A.areEqual('y', outSpec.x);
},

'getMojitTypeDetails caching': function() {
var key = Y.JSON.stringify(['server', ['*'], 'en', 'x']);
store._getMojitTypeDetailsCache[key] = { x: 'y' };
Expand Down
Loading