Relationship Refactor (part 2): The graph should coordinate state updates #7493
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
runspired
changed the base branch from
runspired/simpler-relationships
to
master
|
Asset Size Report for a34fcdf IE11 Builds ✅ EmberData shrank by -3.6 KB (-63.0 B compressed)Warnings
Changeset Full Asset Analysis (IE11)Modern Builds ☑️ EmberData shrank by -2.57 KB but the compressed size increased slighty (+132.0 B compressed)Warnings
Changeset Full Asset Analysis (Modern)Modern Builds (No Rollup) ☑️ EmberData shrank by -370.0 B but the compressed size increased slighty (+491.0 B compressed)Warnings
Changeset Full Asset Analysis (Modern) |
|
Performance Report for a34fcdf Scenario - materialization: ✅ Performance improved
Scenario - unload: ✅ Performance improved
Scenario - destroy: ✅ Performance improved
Scenario - add-children: ✅ Performance improved
Scenario - unused-relationships: ✅ Performance improved
|
runspired
force-pushed
the
runspired/even-simpler-relationships
branch
from
7bd2e41
to
caffec6
Compare
runspired
changed the title
runspired
changed the base branch from
master
to
runspired/simpler-relationships
runspired
force-pushed
the
runspired/even-simpler-relationships
branch
from
0683d69
to
72e8068
Compare
runspired
changed the title
runspired
added
the
🏷️ chore
runspired
force-pushed
the
runspired/even-simpler-relationships
branch
from
84a8e30
to
fb7f032
Compare
snewcomer
approved these changes
May 4, 2021
| this._internalModel.rollbackAttributes(); | ||
| this.store._backburner.join(() => { | ||
| this._internalModel.rollbackAttributes(); | ||
| }); |
There was a problem hiding this comment.
What did you see here (and in other spots) to add the join?
There was a problem hiding this comment.
I noticed we were using a join around the notify and push schedules, which meant these things were being called without an existing join. I worked through all the possible entry points to that flush and found which ones didn't have a join wrapper already and added them.
Moving the join up means that a lot of scenarios that were creating a flush-per-schedule now create only one flush, which helps us avoid a lot more work. This had the most noticeable effect on teardown/unload.
| syncRelationships is used by the UI to grab updates from the graph | ||
| and update the ManyArrays. | ||
|
|
||
| We may be able to remove this once the new relationship layer is | ||
| complete. |
There was a problem hiding this comment.
Related to the other question, what will allow it's removal?
There was a problem hiding this comment.
Our scheduler right now does three things (which is why there are three queues)
- it batches payloads that have been pushed for a single flush, thereby allowing us to order the pushes by type (more efficient to push hasMany first. Removing this batching regressed the update scenario by 500%). It's unclear how we'd do this without some form of run loop concept, however we could probably make a simpler queue thing and drop our need for backburner if this was all that was left.
- it batches (and minimizes) updates to hasMany local state in a second pass, this allows us to update the local state with those changes only once instead of potentially many times. This optimization would go away if we were to compute localState lazily on access.
- it notifies the UI layer only once all state has settled (and minimizes those notifications to only one per relationship). We can possibly do away with this once we no longer support sync-observers if state is also lazily calculated.
This was referenced May 4, 2021
igorT
reviewed
May 5, 2021
| assert.equal(comment.get('post'), post); | ||
| assert.equal(post.get('bestComment'), comment); | ||
| assert.strictEqual(post2.get('bestComment'), null); | ||
| assert.ok(comment.get('post') === post, 'comment post is set correctly'); |
igorT
reviewed
May 5, 2021
| @@ -1046,11 +1046,7 @@ module('integration/store - deleteRecord', function (hooks) { | |||
|
|
|||
| assert.ok(store.hasRecordForId('person', '1'), 'expected the record to be in the store'); | |||
runspired
added
🏷️ perf
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.
Basic premise here is that the individual edges can't optimize updating their inverses, state, or notifying the UI efficiently while the graph, acting as a coordinator, can.
The existing explosion of "remove" and "add" spaghetti (roughly 12 different methods for removing a record from one side last I counted) was primarily due to attempting to prevent "cycling" (an update to one side updating it's inverse which then sends the same updates back to the originating side, potentially bouncing multiple times). These efforts were mostly in vain and we often cycle multiple times even in a first load scenario.
When this PR completes, we should be left in a state of updating the various edges while only visiting each once per relationship payload received (note: a json-api payload will typically contain multiple relationship payloads for the same relationship, so this does not yet do any work to avoid that double pass).