-
Notifications
You must be signed in to change notification settings - Fork 191
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
[BUGFIX] Makes UpdatableTag updates lazy #1010
Merged
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
Currently, if an UpdatableTag is updated with a tag that has a _more recent_ revision value, Glimmer throws an error. This is because the caching strategy that tags use does not have a way to bust caches on updates, without dirtying. However, in real world usage we've encountered a few cases where updating a tag with a more recent version _is_ valid. The use cases we've seen so far are: 1. Updating a ComputedProperty's tag with _lazy dependencies_ which were not used in the original computation: emberjs/data#6961 2. Consuming an argument for the first time asynchronously, after its tag has already be read by the VM: https://github.com/markcharyk/tag-with-a-tag-repro 3. Calculating the value for a ComputedProperty for the first time asynchronously, after its tag has been read by _observers_: emberjs/ember.js#18678 These cases have one thing in common - the tag updates aren't meant to signal a change in state, but to a value that has already been calculated to another value that has been calculated lazily, such that when the later value updates, the former will also update. I believe currently that this is applicable to all updates, since this is what updating a tag is fundamentally about. Either you're creating a tag tree before the fact (and don't need to worry about cache busting) or you're stitching it together after the fact, but you don't want to affect the outcome either way. \### What's in this PR This PR adds a mechanism that enables this laziness, without causing possible accidental cache busts. The issue it solves is this: ```js let tagA = createTag(); let tagB = createTag(); let snapshotA = value(tagA); dirty(tagB); // tagA.revision === 1, tagB.revision === 2 ``` Let's say we read the value of `tagA`. Now it is cached, and if we were to update it to point to `tagB`, it would still return the cached value: ```js validate(tagA, snaphotA); // true update(tagA, tagB); validate(tagA, snaphotA); // true, cache was not busted ``` Now, we create and dirty a third tag that is unrelated, `tagC`. This busts the cache for all tags, and the next time we attempt to validate `tagA` against a previous snapshot, it will return _false_, since it will now return the value of `tagB`: ```js let tagC = createTag(); dirty(tagC); validate(tagA, snaphotA); // false, even though tagA and tagB haven't changed! ``` To prevent this, this PR adds a cache of the value of `tagB` that is checked instead. IFF `tagB`'s value has changed, it will propogate that change, otherwise it will return the previous value, so any existing snapshots will remain valid.
rwjblue
reviewed
Jan 22, 2020
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.
I'm still reasoning through this, but IMHO we definitely need a test here...
Performance tests show no statistically significant change: results.pdf |
krisselden
approved these changes
Jan 24, 2020
hjdivad
added a commit
to emberjs/data
that referenced
this pull request
Jan 24, 2020
Test is automatically re-enabled for 3.17.x. We can re-enable it once the upstream failure (glimmerjs/glimmer-vm#1010) is resolved and into ember-source.
hjdivad
added a commit
to emberjs/data
that referenced
this pull request
Jan 24, 2020
Test is automatically re-enabled for 3.17.x. We can re-enable it once the upstream failure (glimmerjs/glimmer-vm#1010) is resolved and into ember-source.
rwjblue
approved these changes
Jan 24, 2020
hjdivad
added a commit
to emberjs/data
that referenced
this pull request
Jan 24, 2020
Test is automatically re-enabled for 3.17.x. We can re-enable it once the upstream failure (glimmerjs/glimmer-vm#1010) is resolved and into ember-source.
hjdivad
added a commit
to emberjs/data
that referenced
this pull request
Jan 24, 2020
Test is automatically re-enabled for 3.17.x. We can re-enable it once the upstream failure (glimmerjs/glimmer-vm#1010) is resolved and into ember-source.
hjdivad
added a commit
to emberjs/data
that referenced
this pull request
Jan 24, 2020
Test is automatically re-enabled for 3.17.x. We can re-enable it once the upstream failure (glimmerjs/glimmer-vm#1010) is resolved and into ember-source.
hjdivad
added a commit
to emberjs/data
that referenced
this pull request
Jan 24, 2020
Test is automatically re-enabled for 3.17.x. We can re-enable it once the upstream failure (glimmerjs/glimmer-vm#1010) is resolved and into ember-source.
hjdivad
added a commit
to emberjs/data
that referenced
this pull request
Jan 24, 2020
Test is automatically re-enabled for 3.17.x. We can re-enable it once the upstream failure (glimmerjs/glimmer-vm#1010) is resolved and into ember-source. (cherry picked from commit 10ad35f)
hjdivad
added a commit
to emberjs/data
that referenced
this pull request
Jan 24, 2020
Test is automatically re-enabled for 3.17.x. We can re-enable it once the upstream failure (glimmerjs/glimmer-vm#1010) is resolved and into ember-source. (cherry picked from commit 10ad35f)
hjdivad
added a commit
to emberjs/data
that referenced
this pull request
Jan 24, 2020
Test is automatically re-enabled for 3.17.x. We can re-enable it once the upstream failure (glimmerjs/glimmer-vm#1010) is resolved and into ember-source. (cherry picked from commit 10ad35f)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Currently, if an UpdatableTag is updated with a tag that has a more
recent revision value, Glimmer throws an error. This is because the
caching strategy that tags use does not have a way to bust caches on
updates, without dirtying.
However, in real world usage we've encountered a few cases where
updating a tag with a more recent version is valid. The use cases
we've seen so far are:
not used in the original computation: Build failing with ember beta & canary emberjs/data#6961
tag has already be read by the VM: https://github.com/markcharyk/tag-with-a-tag-repro
asynchronously, after its tag has been read by observers: BUG: attempted to update a tag with a tag that has a more recent revision as its value emberjs/ember.js#18678
These cases have one thing in common - the tag updates aren't meant to
signal a change in state, but to a value that has already been
calculated to another value that has been calculated lazily, such that
when the later value updates, the former will also update.
I believe currently that this is applicable to all updates, since this
is what updating a tag is fundamentally about. Either you're creating a
tag tree before the fact (and don't need to worry about cache busting)
or you're stitching it together after the fact, but you don't want to
affect the outcome either way.
### What's in this PR
This PR adds a mechanism that enables this laziness, without causing
possible accidental cache busts. The issue it solves is this:
Let's say we read the value of
tagA
. Now it is cached, and if we wereto update it to point to
tagB
, it would still return the cached value:Now, we create and dirty a third tag that is unrelated,
tagC
. Thisbusts the cache for all tags, and the next time we attempt to validate
tagA
against a previous snapshot, it will return false, since itwill now return the value of
tagB
:To prevent this, this PR adds a cache of the value of
tagB
that ischecked instead. IFF
tagB
's value has changed, it will propogate thatchange, otherwise it will return the previous value, so any existing
snapshots will remain valid.