Caching stuff #208

scjackson · 2016-01-25T20:36:18Z

Added a number of options/functionality around caching including:

getCacheValues and clearCacheValues to Reactor. This will allow users to monitor their own cache usage and clear it if need be.
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.
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 · 2016-01-25T21:33:47Z

src/getter.js

+ * @param {*} options.cacheKey
+ * @returns {getter}
+ */
+function addGetterOptions(getter, options) {


This API change to introduce the concept of "getter options" is a little awkward imo.

Currently, it will replace any previously set options which could lead to unexpected behavior given its name.
An alternative would be to call this setGetterOptions to indicate it would replace, not patch, the options.
But I feel like the API is being conflated with implementation details of hidden __options object. The alternative that I would suggest is two separate methods to configure these options.

cc @jordangarcia since this is API design considerations

@jordangarcia Another thing to consider is that it's probably natural from an API standpoint that getters are configured where they are defined. It may be time to introduce a formal Getter() constructor... :)

I would like to see a Getter constructor.

// same as `['items']` var getter = Getter(['items']); // or var getter = Getter(['items'], { useCache: false });

@jordangarcia Is there an advantage to using a constructor outside of style preference?

loganlinn · 2016-01-25T22:10:35Z

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.

jordangarcia · 2016-01-25T23:26:58Z

src/reactor.js

+   * Clear all cached values
+   */
+  clearCacheValues() {
+    this.reactorState = this.reactorState.set('cache', Immutable.Map())


what is the use case of this api method?

added as an afterthought, thought it might be useful in some cases of switching views in a spa. will remove

scjackson · 2016-01-26T00:20:00Z

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

scjackson · 2016-01-26T16:56:28Z

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:

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.
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 · 2016-01-26T17:14:49Z

@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 · 2016-01-26T17:21:31Z

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

jordangarcia · 2016-01-26T17:25:47Z

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 · 2016-01-26T17:47:18Z

@jordangarcia So this would include:

alwaysCache, skipCache, and cacheKey options for getters, specified via Getter constructor
Cached values for observed getters are dropped on unobserve (this would help us a lot)
Add a default maxItems limit -- I propose 1000
(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 · 2016-01-26T17:54:36Z

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 · 2016-01-26T19:32:47Z

@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 · 2016-01-26T19:48:59Z

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
#208 (comment)
.

scjackson · 2016-01-28T18:34:39Z

@jordangarcia

Updated with the following:

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

scjackson · 2016-01-28T18:59:37Z

@jordangarcia

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

jordangarcia · 2016-01-29T18:30:31Z

src/getter.js

@@ -9,6 +11,19 @@ import { isKeyPath } from './key-path'
 */
 const identity = (x) => x

+class GetterClass {


I think this would be better as a pseudo-constructor. IE I dont like the idea of changing the Getter type because now we have to handle both a Getter instance and a plain array getter in every interface.

Instead Getter(['items']) returning simply ['items'] makes a lot of sense to me for backwards compat and simplicity.

so basically what i had before?

yes, but called Getter instead of addOptionsToGetter. The latter name implied that you could mutate an existing Getter (potentially globally) versus a function to create a Getter.

Ok -- addOptionsToGetter was by ref. Which would modify it globally. We could return a cloned copy with options added instead. Thoughts?

jordangarcia · 2016-01-29T18:34:02Z

src/reactor/fns.js

-  return observerState.withMutations(map => {
-    entriesToRemove.forEach(entry => removeObserverByEntry(map, entry))
+  // Update both observer and reactor state
+  observerState = observerState.withMutations(oState => {


not a fan of these function params, can we not do observerState because of no-shadow?

Could you clarify? Not sure what you mean by this?

scjackson · 2016-02-01T20:00:58Z

@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.

Sam Jackson added 5 commits January 21, 2016 17:02

adding basic lru logic

bacc9d4

comments/formatting

836b4e6

addGetterOptions

3c940f5

add return type to comment

5351733

clear cache values

da98531

loganlinn reviewed Jan 25, 2016
View reviewed changes

jordangarcia reviewed Jan 25, 2016
View reviewed changes

Sam Jackson added 5 commits January 27, 2016 16:26

Getter, delete from cache on observe

5025e2c

comments

28cc547

more tests, fixes

372f8b7

formatting

fe630e9

adding default max

69e6904

Sam Jackson added 2 commits January 28, 2016 11:23

removing duplicate packages

ce555ed

fixing lint errors

2ea41bb

jordangarcia reviewed Jan 29, 2016
View reviewed changes

loganlinn mentioned this pull request Jan 31, 2016

Caching Abstraction #211

Merged

Sam Jackson added 2 commits February 1, 2016 10:04

removing Getter object

90b35c6

fixing lint errors

5aef5eb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Caching stuff #208

Caching stuff #208

scjackson commented Jan 25, 2016

loganlinn Jan 25, 2016

loganlinn Jan 25, 2016

jordangarcia Jan 25, 2016

scjackson Jan 26, 2016

loganlinn commented Jan 25, 2016

jordangarcia Jan 25, 2016

scjackson Jan 26, 2016

scjackson commented Jan 26, 2016

scjackson commented Jan 26, 2016

jordangarcia commented Jan 26, 2016

scjackson commented Jan 26, 2016

jordangarcia commented Jan 26, 2016

scjackson commented Jan 26, 2016

jordangarcia commented Jan 26, 2016

scjackson commented Jan 26, 2016

jordangarcia commented Jan 26, 2016

scjackson commented Jan 28, 2016

scjackson commented Jan 28, 2016

jordangarcia Jan 29, 2016

scjackson Jan 29, 2016

jordangarcia Jan 29, 2016

scjackson Jan 29, 2016

jordangarcia Jan 29, 2016

scjackson Jan 29, 2016

scjackson commented Feb 1, 2016

Caching stuff #208

Are you sure you want to change the base?

Caching stuff #208

Conversation

scjackson commented Jan 25, 2016

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

loganlinn commented Jan 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scjackson commented Jan 26, 2016

scjackson commented Jan 26, 2016

jordangarcia commented Jan 26, 2016

scjackson commented Jan 26, 2016

jordangarcia commented Jan 26, 2016

scjackson commented Jan 26, 2016

jordangarcia commented Jan 26, 2016

scjackson commented Jan 26, 2016

jordangarcia commented Jan 26, 2016

scjackson commented Jan 28, 2016

scjackson commented Jan 28, 2016

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

Choose a reason for hiding this comment

scjackson commented Feb 1, 2016