Efficiently canonicalize InMemoryCache result objects. #7439
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
benjamn
commented
Dec 9, 2020
src/cache/inmemory/readFromStore.ts
Outdated
| @@ -337,6 +334,8 @@ export class StoreReader { | |||
| return finalResult; | |||
| } | |||
|
|
|||
| private canon = new Canon; | |||
There was a problem hiding this comment.
Since the Canon belongs to the StoreReader, which belongs to the InMemoryCache, unused memory retained by reader.canon can be garbage collected if the cache is discarded.
That said, I'm also contemplating an API to jettison this kind of data without discarding the whole cache, since it can always be recomputed (though the === guarantee will be lost temporarily).
benjamn
force-pushed
the
canonical-cache-results
branch
from
ee867a9
to
f23ffb8
Compare
benjamn
force-pushed
the
canonical-cache-results
branch
from
f23ffb8
to
7ff1b31
Compare
benjamn
force-pushed
the
canonical-cache-results
branch
from
f0e3fd8
to
5266120
Compare
benjamn
force-pushed
the
canonical-cache-results
branch
from
5266120
to
9c6a09c
Compare
hwillson
approved these changes
Dec 16, 2020
There was a problem hiding this comment.
This is AWESOME @benjamn! Just a few comments below (nothing blocking!). Also, we might want to consider adding a few direct ObjectCanon tests, to verify pass and admit. I know we're testing them indirectly, but a few direct explicit tests could help future maintainers ramp up on ObjectCanon functionality more quickly. Thanks very much!
As suggested by @hwillson in this comment: #7439 (comment)
| expect(checkLastResult(bResults, bOyez)).toBe(bOyez); | ||
| expect(checkLastResult(abResults, a456bOyez)).not.toBe(a456bOyez); | ||
| expect(checkLastResult(abResults, a456bOyez)).toBe(a456bOyez); |
There was a problem hiding this comment.
Very Shakespearean of you
benjamn
added a commit
that referenced
this pull request
Mar 23, 2021
The goal of broadcastWatches is to notify cache watchers of any new data resulting from cache writes. However, it's possible for cache writes to invalidate watched queries in a way that does not result in any differences in the resulting data, so this watch.lastDiff caching saves us from triggering a redundant broadcast of exactly the same data again. Note: thanks to #7439, when two result objects are deeply equal to each another, they will automatically also be === to each other, which is what allows us to get away with the !== check in this code.
benjamn
added a commit
that referenced
this pull request
Apr 16, 2021
Revisiting PR #6891, which was reverted by commit c2ef68f. With the introduction of canonical cache results (#7439), the strict equality check in setDiff is about to become mostly synonymous with deep equality checking (but much faster to check), so we need to deal with the consequences of #6891 one way or another. As evidence that this change now (after #7439) has no observable impact, notice that we were able to switch back to using !equal(a, b) without needing to fix/update any failing tests, compared to the few tests that needed updating in #6891. However, by switching to deep equality checking, we allow ourselves the option to experiment with disabling (or periodically clearing) the ObjectCanon, which would presumably lead to broadcasting more !== but deeply equal results to cache.watch watchers. With deep equality checking in setDiff, those !== results will get past the filter in the same cases canonical objects would, so there's less of a discrepancy in client behavior with/without canonization enabled.
This was referenced Apr 16, 2021
benjamn
added a commit
that referenced
this pull request
May 10, 2021
Although cache result canonization (#7439) is algorithmically efficient (linear time for tree-shaped results), it does have a computational cost for large result trees, so you might want to disable canonization for exceptionally big queries, if you decide the future performance benefits of result canonization are not worth the initial cost. Fortunately, this implementation allows non-canonical results to be exchaged later for canonical results without recomputing the underlying results, but merely by canonizing the previous results. Of course, this reuse happens only when the cache has not been modified in the meantime (the usual result caching/invalidation story, nothing new), in which case the StoreReader does its best to reuse as many subtrees as it can, if it can't reuse the entire result tree.
benjamn
added a commit
that referenced
this pull request
May 10, 2021
Although cache result canonization (#7439) is algorithmically efficient (linear time for tree-shaped results), it does have a computational cost for large result trees, so you might want to disable canonization for exceptionally big queries, if you decide the future performance benefits of result canonization are not worth the initial cost. Fortunately, this implementation allows non-canonical results to be exchaged later for canonical results without recomputing the underlying results, but merely by canonizing the previous results. Of course, this reuse happens only when the cache has not been modified in the meantime (the usual result caching/invalidation story, nothing new), in which case the StoreReader does its best to reuse as many subtrees as it can, if it can't reuse the entire result tree.
benjamn
added a commit
that referenced
this pull request
May 12, 2021
Revisiting PR #6891, which was reverted by commit c2ef68f. With the introduction of canonical cache results (#7439), the strict equality check in setDiff is about to become mostly synonymous with deep equality checking (but much faster to check), so we need to deal with the consequences of #6891 one way or another. As evidence that this change now (after #7439) has no observable impact, notice that we were able to switch back to using !equal(a, b) without needing to fix/update any failing tests, compared to the few tests that needed updating in #6891. However, by switching to deep equality checking, we allow ourselves the option to experiment with disabling (or periodically clearing) the ObjectCanon, which would presumably lead to broadcasting more !== but deeply equal results to cache.watch watchers. With deep equality checking in setDiff, those !== results will get past the filter in the same cases canonical objects would, so there's less of a discrepancy in client behavior with/without canonization enabled.
benjamn
added a commit
that referenced
this pull request
May 14, 2021
This reimplementation of the stable `stringify` function used by `getStoreKeyName` builds on the `ObjectCanon` introduced by my PR #7439, which guarantees canonical objects keys are always in sorted order.
This was referenced Sep 15, 2021
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.
As explained in #4141 (comment), this change guarantees that any two result objects returned by
InMemoryCachewill be referentially equal (===) if they are deeply equal (note: the reverse is trivially true), which is an extremely valuable property for UI components that can skip re-rendering when the current input data are identical to the previous data.In particular, this change means there will be no difference between optimistic mutation results and non-optimistic final results, as long as the optimistic guess was accurate, fixing #4141 and a number of other similar issues.