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

Add options to cache.gc to enable deleting nonessential/recomputable result caching data #8421

Merged
merged 10 commits into from
Jul 6, 2021

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jun 22, 2021

Ever since #3394, InMemoryCache has taken an approach of trading memory for improved efficiency of repeated cache reads.

More recently (as of #7439), this result caching system also guarantees cache results will be canonical, a technique that uses memory to store previous canonical objects in an efficient lookup structure (the ObjectCanon), to speed up future deep equality comparisons.

Although we continue to believe this memory/speed tradeoff is appropriate in most cases, it is conceivable that you might at some point need to reclaim memory more urgently than you need cache performance.

To release all cached result objects (to be garbage collected by JavaScript if otherwise unused) without resetting any actual cache data, this PR allows calling the cache.gc (#5310) method with a new resetResultCache: true option:

cache.gc({
  resetResultCache: true,
});

By default, resetting the result cache will not reset the ObjectCanon, which provides === continuity of cache results even after the result caching system has been completely replaced (as it is when you pass resetResultCache: true to cache.gc). That preservation is pretty nifty, but if you also want to dump the ObjectCanon for maximum memory savings, then you should also pass resetResultIdentities: true to cache.gc:

cache.gc({
  resetResultCache: true,
  resetResultIdentities: true,
});

These cache.gc options are available only on InMemoryCache (not on ApolloCache), since result caching is an InMemoryCache-specific concept.

The resetResultCache and resetResultIdentities options were the first two options that seemed useful to me, but we can easily add additional options in the future.

@benjamn benjamn changed the title Add options to cache.gc to enable deleting nonessential/recomputable result caching data Add options to cache.gc to enable deleting nonessential/recomputable result caching data Jun 23, 2021
benjamn added a commit that referenced this pull request Jun 23, 2021
@benjamn benjamn force-pushed the cache.gc-options.resetResultCache branch from 0c9dfb7 to d845072 Compare June 23, 2021 19:21
benjamn added a commit that referenced this pull request Jun 23, 2021
@benjamn benjamn force-pushed the cache.gc-options.resetResultCache branch from d845072 to 07e84a0 Compare June 23, 2021 19:33
Copy link
Contributor

@brainkim brainkim left a comment

Choose a reason for hiding this comment

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

Wrote some thoughts. Again I’m not 100% familiar with how advanced caching stuff is supposed to work so my input is limited. My primary concerns are about the API design. Two new boolean options, which seem to be interrelated, and for an advanced use-case which might hard to search for/understand as a non-Apollo Client maintainer. Maybe we can loop in some advanced users like @sofianhn and the documentation team @StephenBarlow for their thoughts?

Also, if there are any relevant issues from users asking for this that I missed I’d love to see them as this could help with the design process.

src/cache/inmemory/inMemoryCache.ts Outdated Show resolved Hide resolved
src/cache/inmemory/inMemoryCache.ts Outdated Show resolved Hide resolved
src/cache/inmemory/inMemoryCache.ts Outdated Show resolved Hide resolved
src/cache/inmemory/readFromStore.ts Show resolved Hide resolved
src/cache/inmemory/inMemoryCache.ts Show resolved Hide resolved
@benjamn
Copy link
Member Author

benjamn commented Jun 28, 2021

@brainkim Thanks for the review! I think I've addressed your concerns, including writing a FinalizationRegistry-based test that was initially failing before I fixed the bug it caught, so thanks again for that reminder/suggestion.

benjamn added 5 commits July 6, 2021 14:17
@benjamn benjamn force-pushed the cache.gc-options.resetResultCache branch from 9d8c3f3 to bb3daf0 Compare July 6, 2021 18:22
@hwillson hwillson merged commit c6dc503 into release-3.4 Jul 6, 2021
@hwillson hwillson deleted the cache.gc-options.resetResultCache branch July 6, 2021 18:25
@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.

4 participants