Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Depend on existence of enclosing entity object when reading from cache. #8147

Merged
merged 4 commits into from
May 11, 2021

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented May 5, 2021

I asked @sofianhn to split up PR #8107 in #8107 (comment), promising a simpler approach to the part unrelated to resultCacheMaxSize (the part about invalidating the executeSelectionSet result cache when entities are evicted).

After some investigation, I believe we can implement most of this functionality in terms of our existing dependency system, as long as cached methods like executeSelectionSet and executeSubSelectedArray register appropriate dependencies.

As luck would have it, we were already tracking an internal __exists dependency for every normalized object in the EntityStore (see #5828), so I believe we can reuse that machinery, which will save us from having to guess (at the end of the cache.evict method) whether or not the whole object was evicted.

I believe what's already implemented here will be enough to release the invalidated cache results to be garbage collected, but we do have some additional options to improve memory management even further:

@benjamn benjamn added this to the Release 3.4 milestone May 5, 2021
@benjamn benjamn self-assigned this May 5, 2021
Comment on lines +555 to +576
export function maybeDependOnExistenceOfEntity(
store: NormalizedCache,
entityId: string,
) {
if (supportsResultCaching(store)) {
// We use this pseudo-field __exists elsewhere in the EntityStore code to
// represent changes in the existence of the entity object identified by
// entityId. This dependency gets reliably dirtied whenever an object with
// this ID is deleted (or newly created) within this group, so any result
// cache entries (for example, StoreReader#executeSelectionSet results) that
// depend on __exists for this entityId will get dirtied as well, leading to
// the eventual recomputation (instead of reuse) of those result objects the
// next time someone reads them from the cache.
store.group.depend(entityId, "__exists");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, the EntityStore tries to track dependencies at the field level, so cached result objects don't have to be invalidated if none of their fields have been invalidated, but when the entire object is removed (or added for the first time), that's a more significant event than the removal/addition/modification of individual fields, so maybeDependOnExistenceOfEntity enables consumers to listen specifically for __exists invalidations.

@sofianhn
Copy link
Contributor

sofianhn commented May 5, 2021

ok, I see now how the this change comes together with benjamn/optimism#195. I was under the impression that I need to add a separate dep for each memoized function to register the dataId dependency. It looks like you figured out a way where you don't even need that part and the system should be now fully connected.

I am going to pull all of this down and test it locally and will get back to you tomorrow. Thanks for spending time on this, this looks great!

Comment on lines 103 to 171
this.executeSelectionSet = wrap(options => this.execSelectionSetImpl(options), {
this.executeSelectionSet = wrap(options => {
maybeDependOnExistenceOfEntity(
options.context.store,
options.enclosingRef.__ref,
);
return this.execSelectionSetImpl(options);
}, {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example of calling maybeDependOnExistenceOfEntity to make executeSelectionSet sensitive to the eviction of the enclosing entity object.

Note that options.objectOrReference (the current object we're executing against) may be an object without an ID, but every object in the cache has some enclosing entity object that does have an ID, which is what options.enclosingRef refers to.

If that whole enclosing entity object gets evicted, then this particular invocation of executeSelectionSet will be invalidated, allowing its cached result to be garbage collected.

@sofianhn
Copy link
Contributor

sofianhn commented May 6, 2021

I ported this to my test app, and I definitely see the effect, but I am not seeing all the entries being evicted. The assumption I have is all once the cache data is fully empty then all created entries should be evicted. A couple of notes:

  • If I were to update per the TODO and add "forget" only when storeFieldName is __exists, only a small subset of the entries would be evicted.

this.d.dirty(makeDepKey(dataId, storeFieldName), storeFieldName === "__exists" ? "forget" : null);

  • If I were to call forget regardless on the storeFieldName, then at that point, I would get most of the entires evicted.

I realized I should share that test so you can try it yourself. That app was based on 3.3.1 and had lots of debugging for my use case. I will clean that up, so it's in a shareable state, and hopefully, that would be useful in catching all scenarios.

@benjamn
Copy link
Member Author

benjamn commented May 7, 2021

@sofianhn Just to make sure, did you also apply benjamn/optimism#195 when you were testing? That PR is necessary for the Entry objects to be fully removed from the Entry graph (when you pass "forget" to dep.dirty). Without that PR, the entries get dirtied, so their attached data get discarded/freed, but the Entry objects themselves are still in the LRU cache.

@sofianhn
Copy link
Contributor

sofianhn commented May 7, 2021

Yes, I applied both. That's why I actually saw some entries getting cleaned. Working on a test now that hopefully will capture what I am seeing.

@sofianhn
Copy link
Contributor

sofianhn commented May 7, 2021

@benjamn , I added the tests as #8169. You can see that the executeSubSelectedArray is fully cleared, but not executeSelectionSet.

@sofianhn
Copy link
Contributor

I updated the testing PR to contain the latest changes from optimism 0.16

@benjamn
Copy link
Member Author

benjamn commented May 10, 2021

@sofianhn I think I see what's wrong. This comment is not the full solution, but I wanted to share the news.

When dirtied, Entry objects drop any deps they previously depended on, which makes sense because (until benjamn/optimism#195) the only thing a dep could do was dirty the Entry. However, after benjamn/optimism#195, if an Entry is already dirty by the time we call dep.dirty(key, "forget") (for some dep the Entry depends on), nothing more will happen, because the dependency was lost when the Entry became dirty.

I would say this behavior is a bug in the (recently added) benjamn/optimism#195 functionality. I'm still thinking through the consequences, and I think there may be a quick/easy solution, but of course we could always switch back to the externally-managed getKey/forgetKey solution.

@benjamn benjamn force-pushed the invalidate-result-cache-on-entity-eviction branch from cda7576 to 457dc65 Compare May 11, 2021 16:31
@benjamn
Copy link
Member Author

benjamn commented May 11, 2021

Since the Netlify previews don't run for PRs not targeting main, I'm going to go ahead and merge this into release-3.4, where I will confirm the appropriate documentation updates took effect.

@benjamn benjamn merged commit 94b7472 into release-3.4 May 11, 2021
@benjamn benjamn deleted the invalidate-result-cache-on-entity-eviction branch May 11, 2021 16:36
@hwillson hwillson removed this from the MM-2021-06 milestone Jul 29, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants