-
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
Improve optimistic update performance by limiting cache key diversity. #5648
Merged
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
hwillson
approved these changes
Dec 3, 2019
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.
Very interesting solution @benjamn (and thanks for explaining the cache key diversity issue so clearly!).
benjamn
force-pushed
the
ignore-previousResult-when-reading-from-cache
branch
from
December 3, 2019 20:13
f9c6d60
to
8c622bb
Compare
benjamn
changed the base branch from
ignore-previousResult-when-reading-from-cache
to
release-3.0
December 3, 2019 20:16
The result caching system introduced by #3394 gained the ability to cache optimistic results (rather than just non-optimistic results) in #5197, but since then has suffered from unnecessary cache key diversity during optimistic updates, because every EntityStore.Layer object (corresponding to a single optimistic update) counts as a distinct cache key, which prevents cached results from being reused if they were originally read from a different Layer object. This commit introduces the concept of a CacheGroup, store.group, which manages dependency tracking and also serves as a source of keys for the result caching system. While the Root object has its own CacheGroup, Layer objects share a CacheGroup object, which is the key to limiting diversity of cache keys when more than one optimistic update is pending. This separation allows the InMemoryCache to enjoy the full benefits of result caching for both optimistic (Layer) and non-optimistic (Root) data, separately.
Although all the components of a key returned by makeCacheKey are important, there's usually one that "anchors" the rest, because it survives the longest and changes the least. In the case of the result caching system, the current EntityStore object is that anchor. This commit formalizes that anchoring role by making the EntityStore literally responsible for generating cache keys based on query AST objects, variables, and other frequently changing inputs. Especially given the recent introduction of CacheGroup logic, it's easier to reason about the makeCacheKey functions if we put part of their implementation behind an abstraction. This abstraction also means we no longer need to pass a KeyTrie into the StoreReader and StoreWriter constructors, as the KeyTrie (context.store.group.keyMaker) now resides within the EntityStore, which is provided as part of the ExecContext (context.store).
benjamn
force-pushed
the
reduce-result-caching-key-diversity
branch
from
December 3, 2019 20:21
ec6ab66
to
0c26be9
Compare
benjamn
changed the title
Improve optimistic result caching by limiting cache key diversity.
Improve optimistic update performance by limiting cache key diversity.
Dec 5, 2019
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 PR restores the full benefits of result caching (#3394) when optimistic updates are pending, which should significantly improve the performance of optimistic updates. Apollo Client 2.x did not attempt to apply result caching to optimistic data, and the
EntityStore
(fkaEntityCache
; see #5618) introduced by Apollo Client 3.0 has been getting the cache key logic slightly wrong since it was first implemented, so the problem addressed by this PR falls somewhere between a bug and an optimization opportunity.Details can be found in the commit messages, reproduced here:
Divide
EntityStore
into optimistic and non-optimisticCacheGroup
sThe result caching system introduced by #3394 gained the ability to cache optimistic results (rather than just non-optimistic results) in #5197, but since then has suffered from unnecessary cache key diversity during optimistic updates, because every
EntityStore.Layer
object (corresponding to a single optimistic update) counts as a distinct cache key, which prevents cached results from being reused if they were originally read from a differentLayer
object.This commit introduces the concept of a
CacheGroup
,store.group
, which manages dependency tracking and also serves as a source of keys for the result caching system. While theRoot
object has its ownCacheGroup
,Layer
objects share aCacheGroup
object, which is the key to limiting diversity of cache keys when more than one optimistic update is pending.This separation allows the
InMemoryCache
to enjoy the full benefits of result caching for both optimistic (Layer
) and non-optimistic (Root
) data, separately.Implement
EntityStore#makeCacheKey
to simplify result cachingAlthough all the components of a key returned by
makeCacheKey
are important, there's usually one that "anchors" the rest, because it survives the longest and changes the least. In the case of the result caching system, the currentEntityStore
object is that anchor.This commit formalizes that anchoring role by making the
EntityStore
literally responsible for generating cache keys based on query AST objects, variables, and other frequently changing inputs.Especially given the recent introduction of
CacheGroup
logic, it's easier to reason about themakeCacheKey
functions if we put part of their implementation behind an abstraction. This abstraction also means we no longer need to pass aKeyTrie
into theStoreReader
constructor, as theKeyTrie
(context.store.group.keyMaker
) now resides within theEntityStore
, which is provided toStoreReader
as part of theExecContext
(context.store
).