Getter dependencies not being cached

[henrist]

When evaluating dependencies, the new reactorState from these evaluations are being discarded, causing the updated cache to also be discarded. This causes a cache miss each time the dependency getter is being used, e.g. by other getters, even though no data has changed.

This should be obvious by looking at this line:

nuclear-js/src/reactor/fns.js

Line 343 in 1444ce6

const args = getDeps(keyPathOrGetter).map(dep => evaluate(reactorState, dep).result)

[mindjuice]

Sorry, I don't see what you mean. That line triggers recursive calls to evaluate() for each dependency. Each call to evaluate() should cache the result when cacheValue() is called on line 348, no?

[mindjuice]

Having said that, I am experiencing problems where it seems that some of my getter functions are being called when no dependencies seem to have changed.

[mindjuice]

Well, I found the source of my issue.

My getDataBindings() function was returning a getter that was created as a function like this:

getDataBindings() {
  return {
    sortedData: SomeModule.Getters.sortedData('asc')
  };
}

Every time my component mounted, getDataBindings() was called and I got a new getter instance, so the hash code for the getter changed. I've moved the bindings to a separate variable and now getDataBindings() returns the same object all the time and things started working much better:

var staticBindings = {
  sortedData: SomeModule.Getters.sortedData('asc')
};

getDataBindings() {
  return staticBindings;
}

The values are definitely being cached for me by Nuclear.

[mindjuice]

Not sure if this is related to @henrist's reported issue, but I was running on 1.1.1, but when my auto-deploy ran, the package.json setting was ^1.1.1 and it installed 1.3.

With 1.1.1, everything works nicely. With 1.3, my getters are constantly being re-evaluated and everything is extremely slow.

I'll be looking into what changed between 1.1.1 and 1.3 on Monday.

[jordangarcia]

That seems concerning. Let me know what you come up with and we will get a
fix out asap.

On Fri, Feb 12, 2016 at 7:55 PM, Ken Carpenter notifications@github.com
wrote:

Not sure if this is related to @henrist https://github.com/henrist's
reported issue, but I was running on 1.1.1, but when my auto-deploy ran,
the package.json setting was ^1.1.1 and it installed 1.3.

With 1.1.1, everything works nicely. With 1.3, my getters are constantly
being re-evaluated and everything is extremely slow.

I'll be looking into what changed between 1.1.1 and 1.3 on Monday.

—
Reply to this email directly or view it on GitHub
#210 (comment)
.

[henrist]

@mindjuice: Line 348 only has a real effect for the last evaluation in the evaluation "chain", as the reactorState (with new cacheValue) from the intermediate evaluations are discarded. reactorState is immutable, so it should be replaced when evaluating each dependency (line 343), but it is not?

[mindjuice]

Minor update. version 1.1.2 works properly. Version 1.2.0 does not.

I'll continue to look into this as time permits today.

Not sure yet if the issue reported by @henrist is related, but could certainly be the same thing.

[mindjuice]

Sorry, I haven't had a chance to look at this again yet.

Side note: I'm in SF all week and just drove by your head office (which made me feel guilty about not having time to look into this yet). Looks like a nice office! It's only a few blocks away from the Arista Networks office (near Twitter at Market and Polk).

[jordangarcia]

Ah no way, let me know if you want to come by and chat.

Shoot me an email @ jordan@optimizely.com

On Tuesday, February 16, 2016, Ken Carpenter notifications@github.com
wrote:

Sorry, I haven't had a chance to look at this again yet.

Side note: I'm in SF all week and just drove by your head office (which
made me feel guilty about not having time to look into this yet). Looks
like a nice office! It's only a few blocks away from the Arista Networks
office (near Twitter at Market and Polk).

—
Reply to this email directly or view it on GitHub
#210 (comment)
.

[mindjuice]

A bit more investigation last night. I found that the entries seem to be getting cached properly, but the code that checks if storeId === stateId always returns false, because the storeId is 1 or 2 bigger than the stateId. This is in the .every() function below:

function isCached(reactorState, keyPathOrGetter) {
  const entry = getCacheEntry(reactorState, keyPathOrGetter)
  if (!entry) {
    return false
  }

  return entry.get('storeStates').every((stateId, storeId) => {
    return reactorState.getIn(['storeStates', storeId]) === stateId
  })
}

Going to keep looking into this, but wanted to post this in case it gives anyone an idea of either what my code could be doing wrong or what the bug might be if it's in Nuclear itself.

[mindjuice]

Had a chat with Jordan today about this. Pretty sure the issue (in my case at least -- sorry for hijacking @henrist's issue) is the new storeId checking code in 1.2.0.

When I trigger an action that updates a field of a store, say the field foo, the storeId is incremented. Then when dependencies for a getter are being checked in isCached(), the .every() code is always failing because the cached data is based on an older version of the store, so the assumption in the code is that the getter must need to be recalculated. In fact, it doesn't need to be. Changing foo should not result in running the getter function of any getter that doesn't depend on foo.

As I discussed with Jordan, I don't think that checking if the store changed to decide if the getters need to be rerun makes sense. The top level store reference can change, but still internally reference (at least some of) the same sub-objects as the previous version of the store. That's a benefit of ImmutableJS' structural sharing/persistent data structures.

When checking dependencies for getter functions in evaluate(), it should only depend on comparing the ImmutableJS references of the dependencies that were previously passed to the getter function to the current references. If any of the dependencies' references are different, then you need to rerun the getter function. If they are the same, then you return the cached result.

For now I will stick with 1.1.2, but I'll try to take a look at how to fix this if I get some time next week (unless someone else beats me to it).

Let me know if you have any thoughts on the above or if you think I am totally wrong.

[loganlinn]

@mindjuice Not needing to check storeId makes sense to me if we check for reference equality, but this sounds like it might be difficult to not leak memory.

[mindjuice]

@loganlinn Oh? How so? You're already caching the last result from each getter, right? When it changes, you replace that reference with the new one, thereby making the old one eligible for garbage collection.

[loganlinn]

@mindjuice In addition to a reference to the previous evaluation result, you also need to hold reference to each dependency of getter. But on second thought, it shouldn't be an issue if done carefully.

[patlillis]

Anyone find a good workaround for this issue?