[perf] disabling _populateCache() in loader in favor of an on-demand process to create internal module info based on the raw meta when the module is needed #1581

Merged
merged 21 commits into from Mar 28, 2014

Conversation

Projects
None yet
5 participants
@caridy
Owner

caridy commented Jan 24, 2014

Initial analysis:

For a very basic page with all necessary modules in the page upfront, and a YUI().use(‘json-stringify’) call, will run under 20ms in chrome/desktop, but it takes more than 100ms in safari iPad3.

For a more complex example, adding more modules (app level modules), we can clearly see more degradation of the initialization, even though none of those modules are "used". Here is an example using modules from screen.yahoo.com (almost 400 new modules + the 407 modules from yui core):

http://jsbin.com/iPOriSAS/1/edit

For what we can see in the profiling, _populateCache(), which is executed once per Y instance, it will do a one time operation to digest all the raw metadata from YUI.Env.modules, passing them thru addModule() method that will do a basic expansion of the meta without doing the recursive computation to expand the dependencies, that happens on demand.

By using the loader built of this branch, we seed a good deal of improvement, and overall a constant time to use despited the number of extra modules added thru the metas, which is exactly what we are looking for.

Changes:

Loader keeps a member called moduleInfo, which is a hash with the internal meta for every module. This structure is used within the loader and from yui.js as well. This PR is proposing a shim on top of that structure to se can populate the cache for each individual module from the raw meta structure YUI.Env.modules when we needed. This will help to not have to walk thousand of modules as part of the initialization of the first YUI instance.

A quirk on this new way of initialization is that conditional modules are not requested by any other module in the require chain, which means they will have to be analyzed before applying any operation to compute dependencies, for that, we are just warming up the conditional cache (which is normally 16 entries for yui core), instead of just initializing all core modules (400+).

Performance tests

command: yb src/loader/tests/performance/loader-tests.js --phantom --ref v3.14.1 --ref v3.15.0

loader resolve core modules

┌───────────┬───────────────────────────┬───────────────────────────────────┬───────────────────────────┐
│           │  Safari (7.0.1) / Mac OS  │  Chrome (33.0.1750.117) / Mac OS  │  Firefox (27.0) / Mac OS  │
├───────────┼───────────────────────────┼───────────────────────────────────┼───────────────────────────┤
│  v3.14.1  │  44.788  ±3.7%            │  44.546  ±3.8%                    │  21.736  ±4.6%            │
│  Working  │  189.414  ±4.4%  +323%    │  127.738  ±8.9%  +187%            │  86.822  ±3.8%  +299%     │
│  v3.15.0  │  157.768  ±3.9%  +252%    │  109.642  ±4.7%  +146%            │  80.497  ±3.9%  +270%     │
└───────────┴───────────────────────────┴───────────────────────────────────┴───────────────────────────┘

loader resolve application modules

┌───────────┬───────────────────────────┬───────────────────────────────────┬───────────────────────────┐
│           │  Safari (7.0.1) / Mac OS  │  Chrome (33.0.1750.117) / Mac OS  │  Firefox (27.0) / Mac OS  │
├───────────┼───────────────────────────┼───────────────────────────────────┼───────────────────────────┤
│  v3.14.1  │  595.568  ±5.7%           │  245.833  ±7.7%                   │  231.745  ±5.0%           │
│  Working  │  6.721k  ±3.3%  +1029%    │  3.450k  ±7.1%  +1304%            │  2.500k  ±4.3%  +979%     │
│  v3.15.0  │  696.895  ±3.0%  +17%     │  279.307  ±4.9%  +14%             │  260.851  ±3.8%  +13%     │
└───────────┴───────────────────────────┴───────────────────────────────────┴───────────────────────────┘

caculate dependecies

┌───────────┬───────────────────────────┬───────────────────────────────────┬───────────────────────────┐
│           │  Safari (7.0.1) / Mac OS  │  Chrome (33.0.1750.117) / Mac OS  │  Firefox (27.0) / Mac OS  │
├───────────┼───────────────────────────┼───────────────────────────────────┼───────────────────────────┤
│  v3.14.1  │  87.696  ±3.8%            │  78.881  ±4.2%                    │  44.247  ±4.4%            │
│  Working  │  306.211  ±3.0%  +249%    │  217.144  ±8.9%  +175%            │  154.839  ±3.1%  +250%    │
│  v3.15.0  │  224.967  ±2.7%  +157%    │  132.550  ±7.0%  +68%             │  110.696  ±3.9%  +150%    │
└───────────┴───────────────────────────┴───────────────────────────────────┴───────────────────────────┘

loader constructor with global cache

┌───────────┬───────────────────────────┬───────────────────────────────────┬───────────────────────────┐
│           │  Safari (7.0.1) / Mac OS  │  Chrome (33.0.1750.117) / Mac OS  │  Firefox (27.0) / Mac OS  │
├───────────┼───────────────────────────┼───────────────────────────────────┼───────────────────────────┤
│  v3.14.1  │  1.159k  ±4.6%            │  402.577  ±4.5%                   │  419.501  ±4.0%           │
│  Working  │  35.813k  ±3.5%  +2990%   │  9.923k  ±7.8%  +2365%            │  12.131k  ±3.6%  +2792%   │
│  v3.15.0  │  1.203k  ±3.0%  +4%       │  406.346  ±4.1%  +1%              │  435.124  ±3.6%  +4%      │
└───────────┴───────────────────────────┴───────────────────────────────────┴───────────────────────────┘

loader constructor without cache

┌───────────┬───────────────────────────┬───────────────────────────────────┬───────────────────────────┐
│           │  Safari (7.0.1) / Mac OS  │  Chrome (33.0.1750.117) / Mac OS  │  Firefox (27.0) / Mac OS  │
├───────────┼───────────────────────────┼───────────────────────────────────┼───────────────────────────┤
│  v3.14.1  │  143.610  ±8.3%           │  127.687  ±5.7%                   │  95.255  ±7.2%            │
│  Working  │  4.830k  ±2.9%  +3263%    │  3.451k  ±12.1%  +2603%           │  2.972k  ±3.7%  +3020%    │
│  v3.15.0  │  138.078  ±8.1%  -4%      │  135.419  ±6.8%  +6%              │  91.891  ±5.6%  -4%       │
└───────────┴───────────────────────────┴───────────────────────────────────┴───────────────────────────┘
@caridy

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Jan 30, 2014

Owner

This is proven to be worth it, bumping the priority of this one. A clean up is needed! /cc @ekashida

Owner

caridy commented Jan 30, 2014

This is proven to be worth it, bumping the priority of this one. A clean up is needed! /cc @ekashida

src/loader/js/loader.js
+ * @param {string} name of the module
+ * @private
+ */
+ _getModuleInfo: function(name) {

This comment has been minimized.

Show comment Hide comment
@ekashida

ekashida Jan 31, 2014

Contributor

Should the accessor to the public moduleInfo property be private?

@ekashida

ekashida Jan 31, 2014

Contributor

Should the accessor to the public moduleInfo property be private?

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Jan 31, 2014

Owner

good question. It seems that moduleInfo was public, but we were not expecting people to use it. That sounds off! http://yuilibrary.com/yui/docs/api/classes/Loader.html#property_moduleInfo

We could make getModuleInfo public, and make moduleInfo private (at least in the docs).

@caridy

caridy Jan 31, 2014

Owner

good question. It seems that moduleInfo was public, but we were not expecting people to use it. That sounds off! http://yuilibrary.com/yui/docs/api/classes/Loader.html#property_moduleInfo

We could make getModuleInfo public, and make moduleInfo private (at least in the docs).

src/loader/js/loader.js
+ var rawMetaModules = META.modules,
+ globalConditions = GLOBAL_ENV._conditions,
+ globalRenderedMods = GLOBAL_ENV._renderedMods,
+ internal = this._internal;

This comment has been minimized.

Show comment Hide comment
@ekashida

ekashida Feb 4, 2014

Contributor

Need to change this semicolon to a comma.

@ekashida

ekashida Feb 4, 2014

Contributor

Need to change this semicolon to a comma.

@ezequiel ezequiel self-assigned this Mar 4, 2014

@ezequiel ezequiel added this to the Sprint 13 milestone Mar 4, 2014

@ezequiel ezequiel added the Loader label Mar 8, 2014

@ezequiel ezequiel removed their assignment Mar 12, 2014

@caridy

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Mar 14, 2014

Owner

This PR is ready for a formal review.

Owner

caridy commented Mar 14, 2014

This PR is ready for a formal review.

src/loader/js/loader.js
- for (i in cache) {
- if (cache.hasOwnProperty(i)) {
- self.moduleInfo[i] = Y.merge(cache[i]);
+ _getModuleInfo: function(name) {

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Mar 14, 2014

Owner

This is the new accessor for module info, although, in some places we are intentionally using the internal hash this.moduleInfo to avoid the penalty of the function call, even though there is a short circuit in this method for that case. Ideally, we can test this and simply drop the property in favor of the accessor, or we can replace the property with a define property (when that feature is available), and that will help us to control this better.

@caridy

caridy Mar 14, 2014

Owner

This is the new accessor for module info, although, in some places we are intentionally using the internal hash this.moduleInfo to avoid the penalty of the function call, even though there is a short circuit in this method for that case. Ideally, we can test this and simply drop the property in favor of the accessor, or we can replace the property with a define property (when that feature is available), and that will help us to control this better.

@@ -1227,16 +1258,12 @@ Y.Loader.prototype = {
}
if (o.condition) {
- t = o.condition.trigger;

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Mar 14, 2014

Owner

housekeeping on this. I hate the fact that trigger can be an array, I don't think that's useful, but anyhow, I just clean up the code and enable alias for when it is an array.

@caridy

caridy Mar 14, 2014

Owner

housekeeping on this. I hate the fact that trigger can be an array, I don't think that's useful, but anyhow, I just clean up the code and enable alias for when it is an array.

@@ -642,7 +642,8 @@ YUI.add('loader-tests', function(Y) {
cond: {
fullpath: resolvePath('../assets/cond.js'),
condition: {
- trigger: 'yql'
+ trigger: 'yql',
+ when: 'before'

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Mar 14, 2014

Owner

@ezequiel we should proactively work on this and try to see if the issue is related to this PR.

@caridy

caridy Mar 14, 2014

Owner

@ezequiel we should proactively work on this and try to see if the issue is related to this PR.

@ezequiel

This comment has been minimized.

Show comment Hide comment
@ezequiel

ezequiel Mar 21, 2014

Contributor

I think this pull request inadvertently fixes #768 and #769.

Contributor

ezequiel commented Mar 21, 2014

I think this pull request inadvertently fixes #768 and #769.

src/loader/js/loader.js
- if (cache.hasOwnProperty(i)) {
- self.moduleInfo[i] = Y.merge(cache[i]);
+ if (this.moduleInfo[name]) {
+ return this.moduleInfo[name];

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Mar 21, 2014

Owner

You could optimize this lookup by not doing it twice, but once and bind it to a local var.

@ericf

ericf Mar 21, 2014

Owner

You could optimize this lookup by not doing it twice, but once and bind it to a local var.

src/loader/js/loader.js
+ internal = this._internal,
+ v;
+
+ if (globalRenderedMods && globalRenderedMods.hasOwnProperty(name) && !this.ignoreRegistered) {

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Mar 21, 2014

Owner

Code comments here would be helpful the next time we need to look at this code.

@ericf

ericf Mar 21, 2014

Owner

Code comments here would be helpful the next time we need to look at this code.

+ this.moduleInfo[name] = Y.merge(globalRenderedMods[name]);
+ } else {
+ if (rawMetaModules.hasOwnProperty(name)) {
+ this._internal = true; // making sure that modules from raw data are marked as internal

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Mar 21, 2014

Owner

this is the Loader instance, right? What does it mean for it to be "internal"?

@ericf

ericf Mar 21, 2014

Owner

this is the Loader instance, right? What does it mean for it to be "internal"?

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Mar 21, 2014

Owner

yeah, this _internal flag was in place, it is not new. Apparently, addModule() want to know if the module came from a meta module, or if it was added manually thru applyConfig() or just called thru YUI.add(), to figure the value of ext. All these are assumptions though ;)

@caridy

caridy Mar 21, 2014

Owner

yeah, this _internal flag was in place, it is not new. Apparently, addModule() want to know if the module came from a meta module, or if it was added manually thru applyConfig() or just called thru YUI.add(), to figure the value of ext. All these are assumptions though ;)

src/loader/js/loader.js
+ v = this.addModule(rawMetaModules[name], name);
+ // Inspect the page for the CSS module and mark it as loaded.
+ if (v && v.type && v.type === CSS) {
+ if (this.isCSSLoaded(v.name, true)) {

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Mar 21, 2014

Owner

This is doing the trick where it toggles a classname on the DOM node and checks its computed style?

@ericf

ericf Mar 21, 2014

Owner

This is doing the trick where it toggles a classname on the DOM node and checks its computed style?

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Mar 21, 2014

Owner

yeah, that was there before, just moved around since it was checking for all css modules ahead of time to see if they where in the page to set it up the values in cache, now it waits until you try to use each of them to check the value (which implies accessing DOM)

@caridy

caridy Mar 21, 2014

Owner

yeah, that was there before, just moved around since it was checking for all css modules ahead of time to see if they where in the page to set it up the values in cache, now it waits until you try to use each of them to check the value (which implies accessing DOM)

src/loader/js/loader.js
- cache = GLOBAL_ENV._conditions;
+ if (cache && !this.ignoreRegistered) {

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Mar 21, 2014

Owner

Can you add a code comment for what ignoreRegistered means in this context.

@ericf

ericf Mar 21, 2014

Owner

Can you add a code comment for what ignoreRegistered means in this context.

src/loader/js/loader.js
} else {
for (i in defaults) {
- if (defaults.hasOwnProperty(i)) {

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Mar 21, 2014

Owner

defaults means YUI core modules?

@ericf

ericf Mar 21, 2014

Owner

defaults means YUI core modules?

src/loader/js/loader.js
+ t = Y.Array(defaults[i].condition.trigger);
+ for (j = 0; j < t.length; j += 1) {
+ trigger = t[j];
+ if (YUI.Env.aliases[trigger]) {

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Mar 21, 2014

Owner

Code comment for what is means to have a alias for a trigger.

@ericf

ericf Mar 21, 2014

Owner

Code comment for what is means to have a alias for a trigger.

src/loader/js/loader.js
+ trigger = YUI.Env.aliases[trigger];
+ }
+ this.conditions[trigger] = this.conditions[trigger] || {};
+ this.conditions[trigger][defaults[i].name || i] = defaults[i].condition;

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Mar 21, 2014

Owner

this.conditions[trigger] should be cached to a local var when possible to avoid extra lookups.

@ericf

ericf Mar 21, 2014

Owner

this.conditions[trigger] should be cached to a local var when possible to avoid extra lookups.

@ericf

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Mar 21, 2014

Owner

This should be in a 3.x release because it changes the public moduleInfo. We should have a way for people to opt back into eagerly populating moduleInfo.

Owner

ericf commented Mar 21, 2014

This should be in a 3.x release because it changes the public moduleInfo. We should have a way for people to opt back into eagerly populating moduleInfo.

- defaults = META.modules,
- cache = GLOBAL_ENV._renderedMods,
- i;
+ getModuleInfo: function(name) {

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Mar 26, 2014

Owner

Is this staying private even though the method name does not start with an underscore?

@ericf

ericf Mar 26, 2014

Owner

Is this staying private even though the method name does not start with an underscore?

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Mar 26, 2014

Owner

I think it is ok to make it public, as the accessor for module info.

@caridy

caridy Mar 26, 2014

Owner

I think it is ok to make it public, as the accessor for module info.

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Mar 28, 2014

Owner

The API doc comment should be updated then.

@ericf

ericf Mar 28, 2014

Owner

The API doc comment should be updated then.

@ericf

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Mar 26, 2014

Owner

The updates look good. I added a comment about whether getModuleInfo() is meant to be public or private.

Owner

ericf commented Mar 26, 2014

The updates look good. I added a comment about whether getModuleInfo() is meant to be public or private.

- delete mod.langCache;
- delete mod.skinCache;
+ mod.langCache = undefined;
+ mod.skinCache = undefined;

This comment has been minimized.

Show comment Hide comment
@ezequiel

ezequiel Mar 27, 2014

Contributor

Why? Setting a property to undefined is different than deleteing it.

@ezequiel

ezequiel Mar 27, 2014

Contributor

Why? Setting a property to undefined is different than deleteing it.

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Mar 27, 2014

Owner

most js engines will go crazy about deleting a member of an object, part of the micro optimizations they do thru the synthetic classes. Deleting a member of a module meta object will simply throw the class away and will require extra work to access any member.

This is obviously not relevant for dictionaries/hashes, which is not the case here.

@caridy

caridy Mar 27, 2014

Owner

most js engines will go crazy about deleting a member of an object, part of the micro optimizations they do thru the synthetic classes. Deleting a member of a module meta object will simply throw the class away and will require extra work to access any member.

This is obviously not relevant for dictionaries/hashes, which is not the case here.

src/loader/js/loader.js
- mods = [].concat(mods, self.moduleInfo[mod].use);
+ modInfo = self.getModuleInfo(mod);
+ if (modInfo && modInfo.use) {
+ mods = [].concat(mods, modInfo.use);

This comment has been minimized.

Show comment Hide comment
@ezequiel

ezequiel Mar 27, 2014

Contributor
mods = mods.concat(modInfo.use);
@ezequiel

ezequiel Mar 27, 2014

Contributor
mods = mods.concat(modInfo.use);
src/loader/js/loader.js
- delete this.moduleInfo[name];
- delete GLOBAL_ENV._renderedMods[name];
+ this.moduleInfo[name] = undefined;
+ GLOBAL_ENV._renderedMods[name] = undefined;

This comment has been minimized.

Show comment Hide comment
@ezequiel

ezequiel Mar 27, 2014

Contributor

Why? Setting a property to undefined is different than deleteing it.

@ezequiel

ezequiel Mar 27, 2014

Contributor

Why? Setting a property to undefined is different than deleteing it.

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Mar 27, 2014

Owner

yeah, after a second pass on this, I think it is OK to delete it since this is a dictionary/hash object.

@caridy

caridy Mar 27, 2014

Owner

yeah, after a second pass on this, I think it is OK to delete it since this is a dictionary/hash object.

+
+ */
+ if (globalRenderedMods && globalRenderedMods.hasOwnProperty(name) && !this.ignoreRegistered) {
+ this.moduleInfo[name] = Y.merge(globalRenderedMods[name]);

This comment has been minimized.

Show comment Hide comment
@ezequiel

ezequiel Mar 27, 2014

Contributor

Is creating a shallow copy of globalRenderedMods[name] really necessary? Are you afraid someone might modify this object?

@ezequiel

ezequiel Mar 27, 2014

Contributor

Is creating a shallow copy of globalRenderedMods[name] really necessary? Are you afraid someone might modify this object?

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Mar 27, 2014

Owner

this is defensive programming :/, that was the way loader was designed, this is not new, it requires copies of the global modules in case you have multiple YUI instances with different modules definition.

@caridy

caridy Mar 27, 2014

Owner

this is defensive programming :/, that was the way loader was designed, this is not new, it requires copies of the global modules in case you have multiple YUI instances with different modules definition.

+ } else {
+ if (rawMetaModules.hasOwnProperty(name)) {
+ this._internal = true; // making sure that modules from raw data are marked as internal
+ v = this.addModule(rawMetaModules[name], name);

This comment has been minimized.

Show comment Hide comment
@ezequiel

ezequiel Mar 27, 2014

Contributor

At this point, if rawMetaModules actually contains name, can we assume this.addModule(rawMetaModules[name], name) will not return null?

@ezequiel

ezequiel Mar 27, 2014

Contributor

At this point, if rawMetaModules actually contains name, can we assume this.addModule(rawMetaModules[name], name) will not return null?

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Mar 27, 2014

Owner

No, there is a possibility that the mod from rawMetaModules contains a configFn method which could return false, which means the module fails to run in the runtime and the module should not be consider as valid, in which case addModule will return null :/

@caridy

caridy Mar 27, 2014

Owner

No, there is a possibility that the mod from rawMetaModules contains a configFn method which could return false, which means the module fails to run in the runtime and the module should not be consider as valid, in which case addModule will return null :/

src/loader/js/loader.js
+ this._internal = true; // making sure that modules from raw data are marked as internal
+ v = this.addModule(rawMetaModules[name], name);
+ // Inspect the page for the CSS module and mark it as loaded.
+ if (v && v.type && v.type === CSS) {

This comment has been minimized.

Show comment Hide comment
@ezequiel

ezequiel Mar 27, 2014

Contributor

Simplification: v && v.type === CSS

This is logically equivalent, as it does not matter whether type exists or not.

@ezequiel

ezequiel Mar 27, 2014

Contributor

Simplification: v && v.type === CSS

This is logically equivalent, as it does not matter whether type exists or not.

This comment has been minimized.

Show comment Hide comment
@ezequiel

ezequiel Mar 27, 2014

Contributor

...in this case. :)

@ezequiel

ezequiel Mar 27, 2014

Contributor

...in this case. :)

This comment has been minimized.

Show comment Hide comment
@caridy

caridy Mar 27, 2014

Owner

good call, yes, CSS is controlled, I will update it.

@caridy

caridy Mar 27, 2014

Owner

good call, yes, CSS is controlled, I will update it.

@ezequiel

This comment has been minimized.

Show comment Hide comment
@ezequiel

ezequiel Mar 27, 2014

Contributor

👍

Contributor

ezequiel commented Mar 27, 2014

👍

@ericf

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Mar 28, 2014

Owner

What do the coverage numbers look like for the code that was changed in this diff? Since this is loader I want us to be extra cautious. We might want to add some tests, especially for the new getModuleInfo() method.

Also we need may need User Guide updates.

Owner

ericf commented Mar 28, 2014

What do the coverage numbers look like for the code that was changed in this diff? Since this is loader I want us to be extra cautious. We might want to add some tests, especially for the new getModuleInfo() method.

Also we need may need User Guide updates.

@ericf

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Mar 28, 2014

Owner

👍

Owner

ericf commented Mar 28, 2014

👍

@caridy caridy merged commit 0829ca1 into yui:dev-3.x Mar 28, 2014

1 check passed

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