add resultCacheMaxSize #8107

sofianhn · 2021-05-01T23:38:40Z

Per the conversation in apollographql/apollo-feature-requests#289, this PR is proposing two changes:

Add resultCachMaxSize config
This will allow customizing the max size of caches used for resultCaching.
const cache = new InMemoryCache({ resultCachMaxSize: 5000 });
Add automatic clean up for the caches in executeSelectionSet and in executeSubSelectedArray upon eviction of related entries.

apollo-cla · 2021-05-01T23:38:42Z

@sofianhn: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

sofianhn · 2021-05-01T23:40:19Z

patches/optimism+0.15.0.patch

@@ -0,0 +1,47 @@
+diff --git a/node_modules/optimism/lib/bundle.cjs.js b/node_modules/optimism/lib/bundle.cjs.js


the patched package is included here for reference, here is the upstream PR: benjamn/optimism#189

sofianhn · 2021-05-02T00:23:22Z

I added a post install for npx patch-package to get CI to actually do the patching and run the test but it does not seem to have picked that up? Maybe caching the build?

For me running npm test passes locally.

benjamn · 2021-05-03T15:06:39Z

@sofianhn Thanks for this! I/we will take a deeper look this week, but a couple things jumped out at me right away:

Please rebase this branch against release-3.4, since it represents significant new (internal) functionality that we probably do not want to release in a patch version of Apollo Client, and some related parts of the code have changed in v3.4 that might be relevant to your goals.
The name of the option has a small misspelling: shouldn't it be resultCacheMaxSize rather than resultCachMaxSize? You used the second spelling consistently, but I think most users will expect Cache to be spelled with an e.

benjamn · 2021-05-03T16:45:05Z

On second thought @sofianhn, would you mind separating the resultCacheMaxSize changes out into a separate PR (ideally targeting release-3.4)? I think I see a much simpler (less code) way to implement the eviction cleanup, but I don't think that discussion needs to block the resultCacheMaxSize stuff. Looking forward to showing you what I have in mind!

sofianhn · 2021-05-03T16:47:37Z

Awesome, looking forward, I will split this to 2 PRs.

sofianhn · 2021-05-04T00:57:23Z

On second thought @sofianhn, would you mind separating the resultCacheMaxSize changes out into a separate PR (ideally targeting release-3.4)? I think I see a much simpler (less code) way to implement the eviction cleanup, but I don't think that discussion needs to block the resultCacheMaxSize stuff. Looking forward to showing you what I have in mind!

This PR now tracks only adding resultCacheMaxSize. I am seeing the effects of #4141 (comment) and want dig deeper there before I put my second PR.

benjamn · 2021-05-05T15:48:31Z

src/cache/inmemory/inMemoryCache.ts

@@ -285,8 +324,7 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
  }

  public reset(): Promise<void> {
-    this.optimisticData = this.optimisticData.prune();
-    this.data.clear();
+    this.init();


Thanks to this change, resetting the cache now dumps this.storeReader and all other result-caching-related data, and replaces them with empty data structures.

benjamn · 2021-05-05T15:49:51Z

src/cache/inmemory/readFromStore.ts

  constructor(private config: StoreReaderConfig) {
    this.config = { addTypename: true, ...config };
+
+    this.executeSelectionSet = wrap(options => this.execSelectionSetImpl(options), {


I don't think these initializations make sense to move to an init or reset method (like I did in InMemoryCache), since it's simpler to dump/replace the whole StoreReader object, and let the original one be garbage collected.

benjamn

LGTM with a few tweaks!

Give me a day or two to whip up some optimism functionality that should make the rest of your original PR easier from the perspective of InMemoryCache. In particular, I think we can reuse the existing dep<TKey> concept, if we allow dep.forget(key) as well as dep.dirty(key), where forgetting causes the parent Entry to be forgotten (removed from the LRU cache, not just dirtied).

benjamn · 2021-05-05T16:59:17Z

These changes have been published in @apollo/client@3.4.0-beta.24 (just now), FYI.

sofianhn commented May 1, 2021

View reviewed changes

sofianhn changed the title ~~Clear cache on eviction main~~ add resultCachMaxSize & clear result caches on eviction May 2, 2021

sofianhn mentioned this pull request May 2, 2021

Add an option to configure result caches max size apollographql/apollo-feature-requests#289

Closed

add resultCacheMaxSize configuration

482f4d8

sofianhn force-pushed the clear-cache-on-eviction-main branch from 2ecbfb9 to 482f4d8 Compare May 4, 2021 00:47

sofianhn requested a review from StephenBarlow as a code owner May 4, 2021 00:47

sofianhn changed the base branch from main to release-3.4 May 4, 2021 00:48

sofianhn changed the title ~~add resultCachMaxSize & clear result caches on eviction~~ add resultCacheMaxSize May 4, 2021

benjamn added 2 commits May 5, 2021 10:09

Stylistic tweaks.

1a6781a

Make cache.reset() also dump all StoreReader/EntityStore data.

526c940

benjamn reviewed May 5, 2021

View reviewed changes

benjamn added this to the Release 3.4 milestone May 5, 2021

benjamn added 💡 idea 🐎 performance 🐘 memory 💸 caching 🧞‍♂️ enhancement 🧪 has-tests labels May 5, 2021

benjamn approved these changes May 5, 2021

View reviewed changes

Mention PR apollographql#8107 in CHANGELOG.md.

dacbbe0

benjamn merged commit d6ee940 into apollographql:release-3.4 May 5, 2021

benjamn mentioned this pull request May 5, 2021

Allow dep.dirty to invoke Entry methods other than setDirty benjamn/optimism#195

Merged

benjamn mentioned this pull request May 5, 2021

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

Merged

2 tasks

hwillson mentioned this pull request May 18, 2021

Cache modification performance degradation experienced at a certain threshold size of data, possibly related to optimism lib cache size (?). #7544

Closed

hwillson added the 2021-06 label Jul 29, 2021

hwillson removed this from the MM-2021-06 milestone Jul 29, 2021

benjamn added a commit that referenced this pull request Sep 27, 2021

Fix PR #8107 link typo in CHANGELOG.md.

3d1d3a3

benjamn mentioned this pull request Sep 27, 2021

Regression PR 8826 broke cache.reset() #8851

Closed

github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add resultCacheMaxSize #8107

add resultCacheMaxSize #8107

sofianhn commented May 1, 2021

apollo-cla commented May 1, 2021

sofianhn May 1, 2021

sofianhn commented May 2, 2021

benjamn commented May 3, 2021

benjamn commented May 3, 2021

sofianhn commented May 3, 2021

sofianhn commented May 4, 2021

benjamn May 5, 2021

benjamn May 5, 2021

benjamn left a comment •

edited

Loading

benjamn commented May 5, 2021

		@@ -0,0 +1,47 @@
		diff --git a/node_modules/optimism/lib/bundle.cjs.js b/node_modules/optimism/lib/bundle.cjs.js

add resultCacheMaxSize #8107

add resultCacheMaxSize #8107

Conversation

sofianhn commented May 1, 2021

apollo-cla commented May 1, 2021

sofianhn May 1, 2021

Choose a reason for hiding this comment

sofianhn commented May 2, 2021

benjamn commented May 3, 2021

benjamn commented May 3, 2021

sofianhn commented May 3, 2021

sofianhn commented May 4, 2021

benjamn May 5, 2021

Choose a reason for hiding this comment

benjamn May 5, 2021

Choose a reason for hiding this comment

benjamn left a comment • edited Loading

Choose a reason for hiding this comment

benjamn commented May 5, 2021

benjamn left a comment •

edited

Loading