Add an option to configure result caches max size

[sofianhn]

The memoization caches used in maybeBroadcashtWatch, executeSelectionSet and in executeSubSelectedArray uses the default max size in optimism of Math.pow(2, 16).

Those caches will grow based on application usage and in the case of executeSelectionSet and executeSubSelectedArray they will continue to grow until the cache limit is reached since they don't offer a "cleaning mechanism", unlike maybeBroadcastWatch, which explicitly removes cache entries.

Proposal

Add an optional resultCachMaxSize configuration option to customize the max size and reduce the potential max memory impact while retaining the performance benefit of resultCaching.

const cache = new InMemoryCache({ resultCachMaxSize: 5000 });

Here is the proposed change + tests.

[sofianhn]

I am also exploring what it takes for executeSelectionSet and executeSubSelectedArray to clean up cache entries based on upstream signals of eviction/gc. I got a prototype working, but not %100 there test-wise. It will be great to know if this is the right direction.

[brainkim]

Thanks for opening this issue! This was actually something I was looking into while doing some related debugging. Is there a specific motivation for why the default cache sizes are too large? I’m very curious to know what your use-case for this is, and if it’s performance-related or whatever.

[sofianhn]

Thanks @brainkim for the quick response.

My use case is actually memory. For a product that runs for multiple days, we expect users to get fresh content continuously. We started noticing that the caches for executeSelectionSet and executeSubSelectedArray can grow to account for large retained memory. My first impression is that we could have been doing something wrong. Still, upon debugging, I did not find anything out of the ordinary. We missed some KeyArgs field policies that improved re-computation, but more entries are added whenever you get fresh content (essentially queries with new keys). That is how we arrived to reconfigure the cache limit to retain resultCaching with a smaller memory footprint as a compromise.

As I mentioned in my first comment, in a perfect world I would love for this to be "related" to eviction. We can reason about what objects to keep in cache and we might be able to tie those to the cahce entries and remove them upon eviction. Not sure if this is something you considered as well but I can share my prototype if you are interested.

Reading your comment, I also just learned about 4141, seems promising. I will look into that as well to see if it can help us. Funny enough, we were unable to move to 3.4 due to some memory leaks but I will give that a second look.

[sofianhn]

Here is the prototype for the automatic removal of entries based on eviction. I verified it in a test app and in a unit test. It does the intended behavior for my use-case.

It will require a minor addition to the optimism API. Happy to discuss in more detail if this is a direction you want to consider.

[brainkim]

@sofianhn You may be interested in the following issue apollographql/apollo-client#7942. Personally, I’m very eager to get this kind of issue sorted, and connecting the cache.evict() API to the optimism caches is probably a good idea. If you open a PR on apollo-client/optimism and ping me I’ll try to cut through any red tape to get it seen!

[sofianhn]

Hey @brainkim, have you had a chance to look at the PR? Any feedback?

[brainkim]

@sofianhn I pinged @benjamn on slack and I think he has some feedback about the optimism stuff. It’s his library and the code is comfy there so I don’t want to be too insistent. If you want to open a PR against Apollo client with the changes you want (not sure how testing that would work) I’m happy to start the review process there. This is actually something I would want to sneak into 3.4 but that’s up to @benjamn of course.

[sofianhn]

Thanks @brainkim I can open a draft PR against Apollo but that PR will depend on the upstream from Optimism (at least the current approach). I will include a patched package to help get this review started.

If there is any specific concern from @benjamn that I can help with, happy to address them as well.

[sofianhn]

@brainkim, PR added: apollographql/apollo-client#8107

[benjamn]

This was implemented and released in @apollo/client@3.4.0-beta.24, thanks to @sofianhn's PR apollographql/apollo-client#8107. 🎉