Lazy resource store #1195

jarnoux · 2013-07-15T20:48:10Z

This pull request implements an optional lazy resolution of resources that potentially makes the resource store creation independent of the number of dimensions and linear with the number of resources.
It also introduces the ability for plugins to alter configurations

…haker), replaced access to _mojitDetails with _resolveMojit() method that resolves and caches resources at runtime, the 'chosen' member of the resources is not irrelevant since we don't chose any of them. It might overload the Y instance with a few useless addons

…n the plugin and its host when the config is later modified by another plugin

…ve resources

added JSON chached resources on the first request transformed for in loops to index based loops

… have been loaded do we want to reload the resources

…s merely a dead-easy way to support that

…at allows to trigger lazy resolution of resources only if it is truthy

gomezd · 2013-07-15T20:52:45Z

Just a note from the description of this PR: "It also introduces the ability for plugins to alter configurations". This is part of a previous PR:
#1187

…ource_store Conflicts: lib/app/addons/rs/selector.js lib/app/autoload/store.server.js

isao · 2013-07-22T17:59:16Z

This looks good to me. I think some of these changes should be the new default behavior (not a separate code path + config), but drewfish is back tomorrow. We'll get his input before proceeding.

drewfish · 2013-07-23T17:00:33Z

I tried this approach and found that it caused mojito to be slower. The interpretation is that _mojitRVs is so big that it causes a large GC walk which slows things down. (Or, having the large _mojitRVs causes other memory pressure, trigger more GC walks or something like that.)

I'll look at the details later.

jarnoux · 2013-07-23T17:05:41Z

I see, did you try stringifying the mojitRVs like you do for the cache?

jarnoux · 2013-07-23T23:16:20Z

also how much slower?

drewfish · 2013-07-24T18:05:16Z

OK, I found the experiment I did earlier. It's from March 21st this year.

Here's what I did:

Ran a search-app-news search results page and captured the calls to store.expandInstanceForEnv().
Wrote a script that loads those calls and replays them, looking for GC events.
Tweaked the RS to do two types of changes: calculate _mojitoDetails at startup or on demand (AKA lazily), store the _mojitoDetails JSON.stringify()ed or as a raw datastructure.
Ran the script with the four experiments (2 x 2).

Alas, it only captured the GC events of the scripts, not the total runtime. Here are the results:

lazy/stringified - 9 GC events
lazy/raw - 9 GC events
startup/stringified - 5 GC events
startup/raw - 6 GC events

From this I surmised that keeping the source data (_appRVs and _mojitRVs) is a lot of data which causes enough memory pressure to cause more GC runs (and more data to walk on each run).

(I had written all these results into a trello card but alas I can't find it.)

caridy · 2013-07-24T19:54:11Z

Trello Card 432:

https://trello.com/c/Cz5hCgj6/432-search-perf-0-gc-profiling

drewfish · 2013-07-24T20:12:41Z

Just moved the card to the right board, so it's now located at:
https://trello.com/c/Cz5hCgj6/256-search-perf-0-gc-profiling

isao · 2013-07-24T20:38:41Z

this test would be interesting to repeat in node 0.10.x. Before then node would trigger GC "whenever the event loop was idle." But the behavior was removed because in practice, V8 was better at deciding when to GC. http://blog.nodejs.org/2013/03/11/node-v0-10-0-stable/ (search for "idle garbage collection")

I think the count of GC events would not mean GC was required more often, for pre 0.10 versions..

drewfish · 2013-07-25T23:10:09Z

The .chosen for resource versions was added as part of pull #1108 (to fix issue #1107) so we should be careful to preserve that.

drewfish · 2013-08-20T20:56:55Z

lib/app/autoload/store.server.js

@@ -191,6 +192,10 @@ YUI.add('mojito-resource-store', function(Y, NAME) {
            this._appPkg = null;    // metadata about the applicaions's NPM package
            this._specPaths = {};   // spec name: full path

+            // Let's try to be more efficient
+            this._smarterDetails = {}; // MojitType: selector: environment: non-stringified details


This structure is useful for the new algorithm, but should be called something else. It contains resource versions and is used by _resolveVersions() so perhaps it should be called _mojitDetailRVs.

drewfish · 2013-08-21T00:05:31Z

lib/app/autoload/store.server.js

            this._appPkg = null;    // metadata about the applicaions's NPM package
            this._specPaths = {};   // spec name: full path

+            this._mojitDetails = {}; // mojitType: selector: environment: non-stringified details
+            this._mojitDetailsCache = {}; // mojitType+poslString+env : resolved resources


For both of these "environment" is really "affinity" no? If so it'd be clearer to say "affinity" instead.

semantically, it is environment: a request has an environment and goes there to look for resources which have affinities, but are indexed by environment.

"Environment" is traditionally (in mojito code and docs) as things like "development", "production", "staging", etc. "Runtime" is used to describe the client vs. server. "Affinity" is used to describe which runtime a particular piece of code is intended for.

Unless there's a strongly compelling reason to use different semantics here, it is strongly recommended to stay with the existing semantics. (If we want to change it, we should change it everywhere: all code and documentation.) That work (and discussion with the whole mojito team about the value of that work) is beyond the scope of this pull request, so please stick to existing semantics. Consistency aids clarity.

Oh hmm... excepts a lot of places in the resource store "env"/"environment" is used for runtime (that is "client" or "server"). However, each resource has an "affinity" ("client", "server", or "common") for a particular "environment". If a field contains the affinity of a resource it should be labelled as "affinity".

drewfish · 2013-08-21T00:13:53Z

Looks like travis build failed due to environmental issues (bad network) and not anything to do with code quality.

…yui resources too

…eans affinities

drewfish · 2013-08-21T16:26:42Z

lib/app/autoload/store.server.js

         * @method resolveResourceVersions
-         * @return {nothing}
+         * @return {[type]} [description]


Hmmm... this @return is confusing. What is the intended return value of this resolveResourceVersions() method?

nothing, dead code

drewfish · 2013-08-21T18:15:08Z

lib/app/autoload/store.server.js

+        /**
+         * Find a mojit details in the resource store at runtime
+         * @param  {String} type the mojit type
+         * @param  {String} env  the environment: {'client', 'server', 'common'}


I think "environment" here is just "client" or "server".

drewfish · 2013-08-21T18:20:07Z

I've confirmed that this (as of 8e6fc76) doesn't regress issue #1107.

drewfish · 2013-08-21T20:10:18Z

+1

drewfish · 2013-08-21T20:10:45Z

Looks like travis build failed due to environmental issues (bad network) and not anything to do with code quality.

drewfish · 2013-08-21T20:14:44Z

Oh, still need test(s) to make sure that app.json resourceStore.lazyLoad (a) actually causes a lazy load, and (b) an app still works with lazy load.

…, affinity has higher priority over shallowness

drewfish · 2013-08-22T17:27:22Z

One last task. Please add a note in the HISTORY.md file mentioning the new feature. You could add something like this in the "Features" section:

* There is a new `resourceStore.lazyResolve:true` option in `application.json`. 
Normally, the resource store at server start pre-calculates the details for all mojits for all contexts/selectors.
(It's optimized to only do this for dimensions/selectors that are actually used.)
By setting this new option the resource store will skip this calculation at srever start and instead do it at runtime as needed.
This might be a good option to use if you have a large number of dimensions defined in `dimensions.json` and a lot of selectors defined in `application.json`,
yet your app will only serve traffic from a subset of those.

drewfish · 2013-08-22T18:48:22Z

Travis is having issues, but the union of builds for 3cf8896 and 91c6bf2 give full tests coverage and passing tests.

drewfish · 2013-08-22T18:48:31Z

+1

imalberto · 2013-08-22T23:55:27Z

lgtm +1

Lazy resource store

jarnoux and others added 13 commits July 15, 2013 19:59

extracted mojit type resolution

2394b56

added mojit resolution to the runtime

6ce64b2

removed unnecessary parameter

9b5d7ea

removed the config initialization that adds some discinnection betwee…

d511139

…n the plugin and its host when the config is later modified by another plugin

resource store is now building a better structure to store and retrie…

a2a5e55

…ve resources

Replaced previous resource resolution with the faster method,

b56a545

added JSON chached resources on the first request transformed for in loops to index based loops

removed dead code

9de243c

modified the flooding level from resource store so it's bearable

eb30f74

removed dead code

0856ff7

added conditional resources reloading at startup, only if some addons…

09c75ad

… have been loaded do we want to reload the resources

we have made shaker pluggable, which means it supports addons. this i…

b738ee6

…s merely a dead-easy way to support that

added the resourceStore.lazyResolve option in the application.yaml th…

cfa1203

…at allows to trigger lazy resolution of resources only if it is truthy

dgomez added 2 commits July 16, 2013 17:39

Merge branch 'develop' of git://github.com/yahoo/mojito into lazy_res…

534ff8a

…ource_store Conflicts: lib/app/addons/rs/selector.js lib/app/autoload/store.server.js

Fixed merge from upstream develop branch.

29240e9

Filter mojit resoource (mojit dir) using the default selector '*'

e1aea33

drewfish reviewed Aug 20, 2013
View reviewed changes

renaming

b69a26a

variable renaming

0f9e901

drewfish reviewed Aug 21, 2013
View reviewed changes

arnoux added 2 commits August 21, 2013 00:45

variable renaming, added sort as a method to filter by depth for non …

448fcd2

…yui resources too

variable renaming: in the priority algorithm, environments actually m…

f15a8ac

…eans affinities

drewfish reviewed Aug 21, 2013
View reviewed changes

renaming and doc fix

8e6fc76

drewfish reviewed Aug 21, 2013
View reviewed changes

removed the shaker subtype + doc fix

a3e1ddb

arnoux added 3 commits August 21, 2013 21:41

added unit tests for lazy loading

bf15962

fixing the priority algorithm to comply with the semantic: posl, type…

1e0b4f3

…, affinity has higher priority over shallowness

added lazy load option in app.json

9464e9d

arnoux added 2 commits August 22, 2013 18:13

added new feature entry to HISTORY.md

3cf8896

moved the new features description

91c6bf2

drewfish added a commit that referenced this pull request Aug 22, 2013

Merge pull request #1195 from gomezd/lazy_resource_store

4b7da37

Lazy resource store

drewfish merged commit 4b7da37 into YahooArchive:develop Aug 22, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy resource store #1195

Lazy resource store #1195

jarnoux commented Jul 15, 2013

gomezd commented Jul 15, 2013

isao commented Jul 22, 2013

drewfish commented Jul 23, 2013

jarnoux commented Jul 23, 2013

jarnoux commented Jul 23, 2013

drewfish commented Jul 24, 2013

caridy commented Jul 24, 2013

drewfish commented Jul 24, 2013

isao commented Jul 24, 2013

drewfish commented Jul 25, 2013

drewfish Aug 20, 2013

drewfish Aug 21, 2013

jarnoux Aug 21, 2013

drewfish Aug 21, 2013

drewfish Aug 21, 2013

drewfish commented Aug 21, 2013

drewfish Aug 21, 2013

jarnoux Aug 21, 2013

drewfish Aug 21, 2013

drewfish commented Aug 21, 2013

drewfish commented Aug 21, 2013

drewfish commented Aug 21, 2013

drewfish commented Aug 21, 2013

drewfish commented Aug 22, 2013

drewfish commented Aug 22, 2013

drewfish commented Aug 22, 2013

imalberto commented Aug 22, 2013

Lazy resource store #1195

Lazy resource store #1195

Conversation

jarnoux commented Jul 15, 2013

gomezd commented Jul 15, 2013

isao commented Jul 22, 2013

drewfish commented Jul 23, 2013

jarnoux commented Jul 23, 2013

jarnoux commented Jul 23, 2013

drewfish commented Jul 24, 2013

caridy commented Jul 24, 2013

drewfish commented Jul 24, 2013

isao commented Jul 24, 2013

drewfish commented Jul 25, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewfish commented Aug 21, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewfish commented Aug 21, 2013

drewfish commented Aug 21, 2013

drewfish commented Aug 21, 2013

drewfish commented Aug 21, 2013

drewfish commented Aug 22, 2013

drewfish commented Aug 22, 2013

drewfish commented Aug 22, 2013

imalberto commented Aug 22, 2013