Avoid using DepTrackingCache for optimistic reads. #4251
Merged
+508
−311
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This should help with #4210, since temporary optimistic ObjectCache objects will no longer be instanceof DepTrackingCache, so the result caching system will be disabled for those reads, and thus the temporary objects will not be retained in any cache keys: https://github.com/apollographql/apollo-client/blob/7d0ed16a97e57cb3789f352fd4f359eaaa2eec0b/packages/apollo-cache-inmemory/src/readFromStore.ts#L123 https://github.com/apollographql/apollo-client/blob/7d0ed16a97e57cb3789f352fd4f359eaaa2eec0b/packages/apollo-cache-inmemory/src/readFromStore.ts#L144
hwillson
approved these changes
Dec 19, 2018
Although this is a rather drastic option, creating an InMemoryCache that
does not attempt to cache previous results may help reduce memory usage,
at the expense of cache performance on repeated reads:
new InMemoryCache({
resultCaching: false, // defaults to true
})
See this comment by @barbalex for one such situation:
#4210 (comment)
At the very least, this option should help diagnose problems with result
caching, even if it's not the right long-term solution.
Anyone who uses `git add -p` or `git commit -p` to contruct their commits from a careful subset of current changes has been bitten by this thoughtless policy many times. I'm done working around it.
benjamn
force-pushed
the
issue-4210-avoid-caching-optimistic-results
branch
from
c24d3d3
to
f25c0dd
Compare
silentsakky
added a commit
to Zimbra/zm-api-js-client
that referenced
this pull request
Dec 20, 2018
- latest version has a fix for memory leak (apollographql/apollo-client#4251) so try to see if that helps
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This tweak should help with #4210, since temporary optimistic
ObjectCacheobjects will no longer beinstanceof DepTrackingCache, so the result caching system will be disabled for those reads, and thus the temporary cache objects will not be retained in any cache keys.Using a cache type other than
DepTrackingCachedisables result caching by making the all-importantmakeCacheKeyfunctions return falsy (undefined) values here and here. Disabling result caching for these reads makes sense, because once the temporaryObjectCacheis discarded, it will never be used again when reading from the cache, so there was never much benefit to caching these reads, anyway. And without the extra overhead of tracking cache dependencies, these reads (which are almost always cold reads) should be a bit faster, too.A longer-term solution to this problem would incorporate optimistic data into the original
DepTrackingCache, in a way that could be efficiently rolled back, without changing the object identity of the store. In the meantime, this PR should put out the #4210 fire. 🤞