Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Replace Y.JSON.xxx with JSON.xxx for server affinities (faster) #521

Merged
merged 3 commits into from

5 participants

@imalberto
Owner

No description provided.

@caridy

This one is common affinity.

@caridy

Let's just remove these logs, they should not be there, and if they do, they should never be using JSON.stringify, instead they should just pass the object as the first param, and mark it as "debug" to avoid computations when running in prod.

@caridy

This is shipped to the client as well. WE need to keep using Y.JSON here.

@imalberto imalberto - removed debug log statements that should not be using Y.JSON.xx
- restored 'json-xx' module in util.common.js (that is deployed to client)
70dcd4b
@add0n

I guess I'd ask to see the actual perf numbers here around 'faster'. IIRC, the Y.JSON call checks first to see if the platform it is running on has a native JSON call and just returns the value of that.

We just changed all 'JSON' calls to 'Y.JSON' to have consistency across the codebase not too long ago. If there is a huge perf win here, then yes for all 'server' affinities we can make an exception and use the raw 'JSON' call. Otherwise, my vote is to leave this as is.

@imalberto
Owner

Updated with feedback from @caridy

@caridy
Owner

@imalberto looking good now +1
@add0n yes, according to Ric's tests, it is 5x faster :( a ticket for YUI has been raised I believe.

@add0n

5X is certainly worth it.

+1

@mojit0

In my mind this is a concrete example of the value of using an M prefixed namespace and isolating from YUI rather than tying ourselves so tightly to it we become dependent on their bug fix schedule. With an M.JSON wrapper we could fix this bug without affecting our API or changing anything but our internals. What will we do when YUI fixes their issue? Continue to have some code with YUI.JSON and some without? or change it back. Both are poor alternatives to taking ownership of the interface under a Mojito (M) prefix.

@imalberto imalberto merged commit d6c7304 into from
@caridy
Owner

@mojit0 I strongly disagree on that. YUI will continue evolving, and fix should be done in YUI so everyone will get the benefit of it, not only mojito, but on top of that there is nothing that prevent us for adding a new module called 'json-stringify.server.js' in autoload as a replacement of the original one with the fix if it is absolutely needed.

@mojit0

@caridy I didn't mean to suggest that YUI won't evolve. My point was that without an insulating layer we can only evolve at the pace they evolve. I believe that we have evidence already that we may find that having our pace dictated by the YUI team may occasionally be burdensome and require us to do changes, such as this pull request, that are actually counter to our long-term API goals.

@jenny
Owner

We should not merge this pull request. We should provide a workaround to the Y.JSON bug and remove it when we upgrade to a YUI version that contains the fix.

@caridy
Owner

@jenny yes, this was merged to our search-perf branch to be able to evaluate how big is the impact as today. Before we merge back to develop we can clean this up.

@jenny
Owner

Thanks for the clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
4 lib/app/addons/ac/deploy.server.js
@@ -299,9 +299,9 @@ YUI.add('mojito-deploy-addon', function(Y, NAME) {
// Unicode escape the various strings in the config data to help
// fight against possible script injection attacks.
yuiConfigEscaped = Y.mojito.util.cleanse(yuiConfig);
- yuiConfigStr = Y.JSON.stringify(yuiConfigEscaped);
+ yuiConfigStr = JSON.stringify(yuiConfigEscaped);
clientConfigEscaped = Y.mojito.util.cleanse(clientConfig);
- clientConfigStr = Y.JSON.stringify(clientConfigEscaped);
+ clientConfigStr = JSON.stringify(clientConfigEscaped);
initializer = '<script type="text/javascript">\n' +
' YUI_config = ' + yuiConfigStr + ';\n' +
View
1  lib/app/addons/ac/intl.common.js
@@ -36,7 +36,6 @@ YUI.add('mojito-intl-addon', function(Y, NAME) {
* @return {string|Object} translated string for label or if no label was provided an object containing all resources.
*/
lang: function(label, args) {
- //Y.log('lang(' + label + ', ' + Y.JSON.stringify(args) + ') for ' + this.ac.type, 'debug', NAME);
Y.Intl.setLang(this.ac.type, this.ac.context.lang);
var string = Y.Intl.get(this.ac.type, label);
if (string && args) {
View
6 lib/app/addons/rs/config.server.js
@@ -79,7 +79,7 @@ YUI.add('addon-rs-config', function(Y, NAME) {
if (!json) {
try {
contents = libfs.readFileSync(fullPath, 'utf-8');
- json = Y.JSON.parse(contents);
+ json = JSON.parse(contents);
} catch (e) {
throw new Error('Error parsing JSON file: ' + fullPath);
}
@@ -109,7 +109,7 @@ YUI.add('addon-rs-config', function(Y, NAME) {
this._ycbCache[fullPath] = {};
}
- cacheKey = Y.JSON.stringify(ctx);
+ cacheKey = JSON.stringify(ctx);
ycb = this._ycbCache[fullPath][cacheKey];
if (!ycb) {
store.validateContext(ctx);
@@ -224,4 +224,4 @@ YUI.add('addon-rs-config', function(Y, NAME) {
Y.namespace('mojito.addons.rs');
Y.mojito.addons.rs.config = RSAddonConfig;
-}, '0.0.1', { requires: ['plugin', 'oop', 'json-parse', 'mojito-util']});
+}, '0.0.1', { requires: ['plugin', 'oop', 'mojito-util']});
View
6 lib/app/addons/rs/selector.server.js
@@ -65,7 +65,7 @@ YUI.add('addon-rs-selector', function(Y, NAME) {
for (c = 0; c < ctxs.length; c += 1) {
ctx = ctxs[c];
posl = this.getPOSLFromContext(ctx);
- posls[Y.JSON.stringify(posl)] = posl;
+ posls[JSON.stringify(posl)] = posl;
}
ctxs = null; // free a bunch of memory
return Y.Object.values(posls);
@@ -86,7 +86,7 @@ YUI.add('addon-rs-selector', function(Y, NAME) {
part,
parts;
- cacheKey = Y.JSON.stringify(ctx);
+ cacheKey = JSON.stringify(ctx);
posl = this._poslCache[cacheKey];
if (!posl) {
store.validateContext(ctx);
@@ -196,4 +196,4 @@ YUI.add('addon-rs-selector', function(Y, NAME) {
Y.namespace('mojito.addons.rs');
Y.mojito.addons.rs.selector = RSAddonSelector;
-}, '0.0.1', { requires: ['plugin', 'oop', 'json-stringify', 'mojito-util']});
+}, '0.0.1', { requires: ['plugin', 'oop', 'mojito-util']});
View
6 lib/app/addons/rs/yui.server.js
@@ -269,7 +269,7 @@ YUI.add('addon-rs-yui', function(Y, NAME) {
env = evt.args.env,
ctx = evt.args.ctx,
posl = evt.args.posl,
- poslKey = Y.JSON.stringify(posl),
+ poslKey = JSON.stringify(posl),
mojitType = evt.args.mojitType,
ress,
r,
@@ -326,7 +326,7 @@ YUI.add('addon-rs-yui', function(Y, NAME) {
onMojitResourcesResolved: function(evt) {
var env = evt.env,
posl = evt.posl,
- poslKey = Y.JSON.stringify(posl),
+ poslKey = JSON.stringify(posl),
mojit = evt.mojit,
ress = evt.ress,
r,
@@ -633,4 +633,4 @@ YUI.add('addon-rs-yui', function(Y, NAME) {
Y.namespace('mojito.addons.rs');
Y.mojito.addons.rs.yui = RSAddonYUI;
-}, '0.0.1', { requires: ['plugin', 'oop', 'loader-base', 'json-stringify', 'mojito-util']});
+}, '0.0.1', { requires: ['plugin', 'oop', 'loader-base', 'mojito-util']});
View
4 lib/app/addons/view-engines/hb.server.js
@@ -67,7 +67,7 @@ YUI.add('mojito-hb', function(Y, NAME) {
*/
compiler: function (tmpl, callback) {
this._getTemplateObj(tmpl, false, function (err, obj) {
- callback(err, Y.JSON.stringify(obj.raw));
+ callback(err, JSON.stringify(obj.raw));
});
},
@@ -124,4 +124,4 @@ YUI.add('mojito-hb', function(Y, NAME) {
Y.namespace('mojito.addons.viewEngines').hb = HandleBarsAdapter;
-}, '0.1.0', {requires: ['json-stringify']});
+}, '0.1.0', {requires: []});
View
4 lib/app/addons/view-engines/mu.server.js
@@ -85,11 +85,11 @@ YUI.add('mojito-mu', function(Y, NAME) {
compiler: function(tmpl, callback) {
fs.readFile(tmpl, 'utf8', function (err, data) {
- callback(err, Y.JSON.stringify(data));
+ callback(err, JSON.stringify(data));
});
}
};
Y.namespace('mojito.addons.viewEngines').mu = MuAdapter;
-}, '0.1.0', {requires: ['json-stringify']});
+}, '0.1.0', {requires: []});
View
6 lib/app/autoload/route-maker.common.js
@@ -26,10 +26,6 @@ YUI.add('mojito-route-maker', function(Y, NAME) {
function resolveParams(route, params) {
- // console.log('============= resolving params for route ' +
- // route.name);
- // console.log(params);
- // console.log('requires: ' + Y.JSON.stringify(route.requires));
var tester = [];
// we don't need to do anything if this route requires no params
@@ -305,7 +301,7 @@ YUI.add('mojito-route-maker', function(Y, NAME) {
return null;
}
- // logger.log('[UriRouter] found route: ' + Y.JSON.stringify(route));
+ // logger.log('[UriRouter] found route: ' + JSON.stringify(route));
match = copy(route);
View
7 lib/store.server.js
@@ -412,7 +412,7 @@ YUI.add('mojito-resource-store', function(Y, NAME) {
k,
use;
- posl = Y.JSON.stringify(this.selector.getPOSLFromContext(ctx));
+ posl = JSON.stringify(this.selector.getPOSLFromContext(ctx));
if (filter.mojit) {
if (!this._mojitResources[env] ||
!this._mojitResources[env][posl] ||
@@ -1306,7 +1306,7 @@ YUI.add('mojito-resource-store', function(Y, NAME) {
for (p = 0; p < posls.length; p += 1) {
posl = posls[p];
- poslKey = Y.JSON.stringify(posl);
+ poslKey = JSON.stringify(posl);
selectors = {}; // selector: priority modifier
for (s = 0; s < posl.length; s += 1) {
selectors[posl[s]] = (posl.length - s - 1) * 2;
@@ -1488,7 +1488,7 @@ YUI.add('mojito-resource-store', function(Y, NAME) {
console.log('--PACKAGE-- ' + info.depth + ' ' + info.pkg.name + '@' + info.pkg.version
+ ' \t' + (info.pkg.yahoo && info.pkg.yahoo.mojito && info.pkg.yahoo.mojito.type)
+ ' \t[' + info.parents.join(',') + ']'
- // + ' \t-- ' + Y.JSON.stringify(info.inherit)
+ // + ' \t-- ' + JSON.stringify(info.inherit)
);
*/
pkg = {
@@ -1967,7 +1967,6 @@ YUI.add('mojito-resource-store', function(Y, NAME) {
}, '0.0.1', { requires: [
'base',
- 'json-stringify',
'oop',
'mojito-util'
]});
Something went wrong with that request. Please try again.