Skip to content
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

Stop paying attention to previousResult in InMemoryCache. #5644

Merged
merged 1 commit into from
Dec 3, 2019

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Dec 2, 2019

The previousResult option was originally a way to ensure referential identity of structurally equivalent cache results, before the result caching system was introduced in #3394. It worked by returning previousResult whenever it was deeply equal to the new result.

The result caching system works a bit differently, and in particular never needs to do a deep comparison of results. However, there were still a few (test) cases where previousResult seemed to have a positive effect, and removing it seemed like a breaking change, so we kept it around.

In the meantime, the equality check has continued to waste CPU cycles, and the behavior of previousResult has undermined other improvements, such as freezing cache results (#4514). Even worse, previousResult effectively disabled an optimization that allowed broadcastWatches to skip unchanged queries (see comments I removed if curious). This commit restores that optimization.

I realized eliminating previousResult might finally be possible while working on PR #5617, which made the result caching system more precise by depending on IDs+fields rather than just IDs. This additional precision seems to have eliminated the few remaining cases where previousResult had
any meaningful benefit, as evidenced by the lack of any test changes in this commit… even among the many direct tests of previousResult in src/cache/inmemory/__tests__/diffAgainstStore.ts!

The removal of previousResult is definitely a breaking change (appropriate for Apollo Client 3.0), because you can still contrive cases where some never-before-seen previousResult object just happens to be deeply equal to the new result. Also, it's fair to say that this removal will strongly discourage disabling the result caching system (which is still possible for diagnostic purposes), since we rely on result caching to get the benefits that previousResult provided. Despite those modest theoretical drawbacks, it would be a relief to be done with previousResult, and I doubt we will find a better opportunity than AC3.

Comment on lines -100 to -105
if (c.previousResult) {
// If a previousResult was provided, assume the caller would prefer
// to compare the previous data to the new data to determine whether
// to broadcast, so we should disable caching by returning here, to
// give maybeBroadcastWatch a chance to do that comparison.
return;
}
Copy link
Member Author

@benjamn benjamn Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the "comments I removed" that I was referring to above. ♻️

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great @benjamn (and I ran RA's tests with these changes as well, just in-case).

@benjamn benjamn force-pushed the allow-evicting-specific-fields branch from 9c33678 to 5f153e8 Compare December 3, 2019 20:10
@benjamn benjamn changed the base branch from allow-evicting-specific-fields to release-3.0 December 3, 2019 20:12
The previousResult option was originally a way to ensure referential
identity of structurally equivalent cache results, before the result
caching system was introduced in #3394. It worked by returning
previousResult whenever it was deeply equal to the new result.

The result caching system works a bit differently, and in particular never
needs to do a deep comparison of results. However, there were still a few
(test) cases where previousResult seemed to have a positive effect, and
removing it seemed like a breaking change, so we kept it around.

In the meantime, the equality check has continued to waste CPU cycles, and
the behavior of previousResult has undermined other improvements, such as
freezing cache results (#4514). Even worse, previousResult effectively
disabled an optimization that allowed InMemoryCache#broadcastWatches to
skip unchanged queries (see comments I removed if curious). This commit
restores that optimization.

I realized eliminating previousResult might finally be possible while
working on PR #5617, which made the result caching system more precise by
depending on IDs+fields rather than just IDs. This additional precision
seems to have eliminated the few remaining cases where previousResult had
any meaningful benefit, as evidenced by the lack of any test changes in
this commit... even among the many direct tests of previousResult in
__tests__/diffAgainstStore.ts!

The removal of previousResult is definitely a breaking change (appropriate
for Apollo Client 3.0), because you can still contrive cases where some
never-before-seen previousResult object just happens to be deeply equal to
the new result. Also, it's fair to say that this removal will strongly
discourage disabling the result caching system (which is still possible
for diagnostic purposes), since we rely on result caching to get the
benefits that previousResult provided.
@benjamn benjamn force-pushed the ignore-previousResult-when-reading-from-cache branch from f9c6d60 to 8c622bb Compare December 3, 2019 20:13
@benjamn benjamn merged commit d809c8f into release-3.0 Dec 3, 2019
@benjamn benjamn mentioned this pull request Dec 3, 2019
31 tasks
@benjamn benjamn deleted the ignore-previousResult-when-reading-from-cache branch December 3, 2019 20:24
benjamn added a commit that referenced this pull request Jan 3, 2020
Fixes #5733, which was caused by multiple watches having the same cache
key, so none except the first were ever re-broadcast, since the first
broadcast had the side effect of marking the later watches as clean,
before this.watches.forEach had a chance to visit them.

Background: PR #5644 reenabled an important optimization for the
InMemoryCache#broadcastWatches method, allowing it to skip watches whose
results have not changed. Unfortunately, while this optimization correctly
determined whether the result had changed, it did not account for the
possibility of multiple distinct consumers of the same result, which can
happen (for example) when multiple components use the same query and
variables via different useQuery calls.

Fortunately, the fix is straightforward (if not exactly obvious): in order
to assign distinct consumers different cache keys, it suffices to include
the provided watch.callback function in the cache key.

A more drastic way to fix #5733 would be to remove the caching of
maybeBroadcastWatch entirely, which would not be a huge loss because the
underlying cache.diff method is also cached. For now, though, with that
backup option in mind, I'd like to preserve this optimization unless it
causes any further problems.
benjamn added a commit that referenced this pull request Jan 3, 2020
Fixes #5733, which was caused by multiple watches having the same cache
key, so none except the first were ever re-broadcast, since the first
broadcast had the side effect of marking the later watches as clean,
before this.watches.forEach had a chance to visit them.

Background: PR #5644 reenabled an important optimization for the
InMemoryCache#broadcastWatches method, allowing it to skip watches whose
results have not changed. Unfortunately, while this optimization correctly
determined whether the result had changed, it did not account for the
possibility of multiple distinct consumers of the same result, which can
happen (for example) when multiple components use the same query and
variables via different useQuery calls.

Fortunately, the fix is straightforward (if not exactly obvious): in order
to assign distinct consumers different cache keys, it suffices to include
the provided watch.callback function in the cache key.

A more drastic way to fix #5733 would be to remove the caching of
maybeBroadcastWatch entirely, which would not be a huge loss because the
underlying cache.diff method is also cached. For now, though, with that
backup option in mind, I'd like to preserve this optimization unless it
causes any further problems.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants