-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Avoid using DepTrackingCache for optimistic reads. #4251
Merged
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch @benjamn!
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
December 19, 2018 19:06
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
ObjectCache
objects 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
DepTrackingCache
disables result caching by making the all-importantmakeCacheKey
functions return falsy (undefined) values here and here. Disabling result caching for these reads makes sense, because once the temporaryObjectCache
is 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. 🤞