Prevent optimistic cache evictions from evicting non-optimistic data. #8829
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
Sep 23, 2021
After thinking further about the discussion in #7321, I believe the inability to roll back optimistic evictions is simply a bug that we can fix, rather than something the developer should have to worry about (e.g. by passing `optimistic: true`, as I previously proposed in #7321 (comment)). Normally, `cache.optimisticData` points to the topmost currently active optimistic `Layer`, and `cache.data` points to the root/non-optimistic layer. While running an optimistic `update` function, we temporarily set `cache.data` to be `=== cache.optimisticData`, in an attempt to confine the optimistic update to one optimistic layer. Eviction was (until now) an exception to this confinement, because optimistic `Layer` objects still have a chain of `layer.parent.parent...` objects that eventually terminates at the `Root` layer. Before now, eviction was blindly following that entire chain, ignoring the current value of `cache.data`. This commit uses `cache.data` as the stopping point for eviction, which has the effect of confining optimistic evictions to the one layer that they should be updating, hopefully solving #7321 without requiring any intervention by the developer.
benjamn
force-pushed
the
better-confinement-for-optimistic-evictions
branch
from
12285a7 to
538fc9c
Compare
hwillson
approved these changes
Sep 26, 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.
After thinking further about the discussion in #7321, I believe the inability to roll back optimistic evictions is simply a bug that we can fix, rather than something the developer should have to worry about (e.g. by passing
optimistic: true, as I previously proposed in #7321 (comment)).Normally,
cache.optimisticDatapoints to the topmost currently active optimisticLayer, andcache.datapoints to the root/non-optimistic layer. While running an optimisticupdatefunction, we temporarily setcache.datato be=== cache.optimisticData, in an attempt to confine the optimistic update to one optimistic layer. Eviction was (until now) an exception to this confinement, because optimisticLayerobjects still have a chain oflayer.parent.parent...objects that eventually terminates at theRootlayer. Before now, eviction was blindly following that entire chain, ignoring the current value ofcache.data.This commit uses
cache.dataas the stopping point for eviction, which has the effect of confining optimistic evictions to the one layer that they should be updating, hopefully solving #7321 without requiring any intervention by the developer. ✨