Caching stuff

[scjackson]

@jordangarcia @mikeng13

Added a number of options/functionality around caching including:

1. getCacheValues and clearCacheValues to Reactor. This will allow users to monitor their own cache usage and clear it if need be.
2. useCache and maxItemsToCache as config options for Reactor. This will allow users to disable caching completely, or limit the number of items that are stored in cache. Items are evicted from cache on a standard LRU policy.
3. A utility function for getters, addOptionsToGetter, which accepts a getter and augments it with options. Currently supported options are useCache, which allows users to specify whether the getter value should be cached on evaluate, and cacheKey, which allows users to specify their own cache key.

[loganlinn]

Rather than exposing crude controls to internal cache and caching behavior, it seems to me a better design would be to introduce a caching interface abstraction to allow the user to customize at will. This would avoid adding complexity to nuclear-js to permit additional caching algorithms (ie maxItemsToCache) and controls (ie clearCacheValues) for this particular use case.

[scjackson]

@loganlinn A cache abstraction would be nice. Did you have something in mind?

[scjackson]

Before I make any changes, I want to make sure I point out the driving motivator of this change, which is that caching as it is currently implemented is broken. There is no cache cleanup, and as such memory will grow linearly over time. This is a real problem for single page apps and data intensive pages. IMO we need to fix or mitigate this out of the box for users.

I'd like to address this in one of two ways:

1. Stick with what I've done above and add a default for maxItemsToCache. This would provide at least some ceiling to memory growth. I'm fine with removing some of the extra surface area I added.
2. Turn caching off by default and remove the LRU stuff.

In either case, adding a cache abstraction would be a logical next iteration to help power users fine tune their apps, but @loganlinn I think we need to address the base case first.

If you have better ideas, or a good handle on what a caching interface would look like, please let me know.

[jordangarcia]

@scjackson I agree that the lack of cache invalidation / cleanup is a core problem that needs to be addressed.

I believe any viable candidate that ships with NuclearJS must (1) prevent the infinitely linear growth of cached data over time and (2) maintain the caching performance benefits that already exist in NuclearJS.

Here is what I propose, when a Getter is unobserved by invoking the returned unwatchFn, if no other observers are using that getter then we remove any cache entries for it. This provides a more natural cache GC when a Getter is no longer "hot".

Thoughts?

[scjackson]

@jordangarcia I like that. Would you be ok with disabling caching for unobserved getters (ie things that are just flux.evaluated)

[jordangarcia]

Hm, good point. I am a little hesitant to change a lot of the out of the box caching behavior as it may have performance impacts on people using it. Imagine a computationally expensive evalaute with a small memory footprint.

I think having the option on a Getter to alwaysCache is reasonable. Getter(['items'], { alwaysCache: true })

The more I think about it, the more I am leaning towards a hybrid approach, where we use intelligent unobserve logic to do cache cleanup and have a pretty high maxItems limit that serves as a cache ceiling for things like flux.evaluate cached Getters.

[scjackson]

@jordangarcia So this would include:

1. alwaysCache, skipCache, and cacheKey options for getters, specified via Getter constructor
2. Cached values for observed getters are dropped on unobserve (this would help us a lot)
3. Add a default maxItems limit -- I propose 1000
4. (Optional) useCache option at the reactor level. Might not be as relevant with the steps taken above, but could be useful for those more concerned with memory usage than speed.

[jordangarcia]

So if we change the default caching behavior of reactor.evaluate to not cache unless alwaysCache is true and we GC cache on Getter unobserve, is there ever a need for skipCache entirely?

The only use case I can think of is if you dont want the getter ever getting into cache during observe, which there may be a case where you want it to be ephemeral (think our results page).

It seems like a getter has 3 caching options, 'always', 'never', and 'default'. I sort of dislike having two flags to represent 3 options. What are your thoughts on

Getter(['items'], {
  cache: 'always',
})

Getter(['items'], {
  cache: 'default',
})

Getter(['items'], {
  cache: 'never',
})

cc: @loganlinn

[scjackson]

@jordangarcia I thought you were advocating for maintaining the cache by default behavior of reactor.evaluate (ie caching by default). One flag seems cleaner to me as well

[jordangarcia]

ah yeah I was unclear on that. If we implement an upper limit as well as an observe cache cleanup then we should be able to keep the same defaults. I'm not committed to one or the other, it'd be good to build the options in and then do some real world testing.

On Tue, Jan 26, 2016 at 11:32 AM, Sam Jackson [email protected] wrote:

@jordangarcia https://github.com/jordangarcia I thought you were advocating for maintaining the cache by default behavior of reactor.evaluate (ie caching by default). One flag seems cleaner to me as well

— Reply to this email directly or view it on GitHub https://github.com/optimizely/nuclear-js/pull/208#issuecomment-175191735 .

[scjackson]

@jordangarcia

Updated with the following:

1. Getter constructor, with cache and cacheKey options
2. Removing unobserved values from cache
3. Dropped getCacheValues and clearCacheValues from Reactor
4. default max items limit of 1000

[scjackson]

@jordangarcia

one other relevant change, I'm now clearing cache values on Reactor.reset as well

[scjackson]

@jordangarcia @loganlinn

Removed the Getter constructor in favor of Getter pseudo constructor, modified removeObserver to only delete cached getter values if nothing else is observing the getter, or if the user invoked remove by getter rather than by entry.