[Fiber] Separate priority field for pending updates #7457
Conversation
ghost
added
acdlite
force-pushed
the
fiberupdatepriority
branch
from
c28caef to
7036ef8
Compare
The priority level gets reset at the wrong time because I rely on mutating it at the wrong point. This moves it to the end of completed work as a second pass over the children to see what the highest priority is. This is inefficient for large sets but we can try to find a smarter way to do this later. E.g. passing it down the stack. This bug fix revealed another bug that I had flagged before that we're finding work to do in the "current" tree instead of the working tree. For trees that were paused, e.g. childInProgress trees, this won't work since don't have a current tree to search. Therefore I fixed findNextUnitOfWorkAtPriority to use workInProgress instead of current.
The findPendingWork phase is the same as just walking the tree the normal way effectively. It only makes things more complex to think about. We might possibly be able to write a few special branches to optimize it but for now it doesn't seem necessary.
I will try a slightly different but similar strategy.
I'll probably end up reverting this before the bigger release since it is not part of the official public API. However, it is useful to be able to compare the performance between functional components and classes.
Updates are scheduled by setting a work priority on the fiber and bubbling it to the root. Because the instance does not know which tree is current at any given time, the update is scheduled on both fiber alternates. Need to add more unit tests to cover edge cases.
Changes the type of pendingState to be a linked list of partial state objects.
Add support for setState((state, props) => newState). Rename pendingState to stateQueue.
Move ReactInstanceMap to src/renderers/shared/shared to indicate that this logic is shared across implementations.
Also cleans up some types.
Callbacks are stored on the same queue as updates. They care called during the commit phase, after the updates have been flushed. Because the update queue is cleared during the completion phase (before commit), a new field has been added to fiber called callbackList. The queue is transferred from updateQueue to callbackList during completion. During commit, the list is reset. Need a test to confirm that callbacks are not lost if an update is preempted.
Adds a field to UpdateQueue that indicates whether an update should replace the previous state completely.
Adds a field to the update queue that causes shouldComponentUpdate to be skipped.
We should be able to abort an update without any side-effects to the current tree. This fixes a few cases where that was broken. The callback list should only ever be set on the workInProgress. There's no reason to add it to the current tree because they're not needed after they are called during the commit phase. Also found a bug where the memoizedProps were set to null in the case of an update, because the pendingProps were null. Fixed by transfering the props from the instance, like we were already doing with state. Added a test to ensure that setState can be called inside a callback.
This is unfortunate since we agreed on using the `null | Fiber` convention instead of `?Fiber` but haven't upgraded yet and this is the pattern I've been using everywhere else so far.
Currently there is a single priority field `pendingWorkPriority` that represents the priority of the entire subtree. The scheduler does a breadth-first search to find the next unit of work with a matching priority level. If a subtree's priority does not match, that entire subtree is skipped. That means when an update is scheduled deep in the tree (`setState`), its priority must be bubbled to the top of the tree. The problem is that this causes the all the parent priorities to be overridden — there's no way to schedule a high priority update inside a low priority tree without increasing the priority of the entire tree. To address this, this PR introduces a separate priority field called `pendingUpdatePriority` (not 100% sold on the name). The new field represents the priority of the pending props and state, which may be lower than the priority of the entire tree. Now, when the scheduler is searching for work to perform, it only matches if the ` pendingUpdatePriority` matches. It continues to use the `pendingWorkPriority` as a way to ignore subtrees if they do not match. (To elaborate further: because the work priority is the highest priority of any node in the entire tree, if it does not match, the tree can be skipped.)
acdlite
force-pushed
the
fiberupdatepriority
branch
from
7036ef8 to
c948425
Compare
|
In the case of down-prioritization, we probably want to down-prioritize all The only time you end up with Therefore, I don't think we actually need a separate field for this other than for state updates. I.e. we could move this field into the Let me know what you think. |
|
You can also end up with |
|
I'll close since this is outdated. I also think it was covered by #8010? |
|
Yeah way outdated |
|
It isn't not covered yet though. In fact the solution to this problem is blocking me from landing the demo properly. You can't really do as interesting scheduling examples without the separate priority field. So we should fix something like it but by putting the field on the update queue. |
|
@sebmarkbage Is this tracked on the umbrella issue? I can pick this back up again, unless you'd rather do it |
|
@acdlite It is not tracked but we probably should. It'd be great if you can pick it back up so we can get the demo with hover effects running smoothly! |
Currently there is a single priority field
pendingWorkPrioritythat represents the priority of the entire subtree. The scheduler does a breadth-first search to find the next unit of work with a matching priority level. If a subtree's priority does not match, that entire subtree is skipped. That means when an update is scheduled deep in the tree (setState), its priority must be bubbled to the top of the tree. The problem is that this causes the all the parent priorities to be overridden — there's no way to schedule a high priority update inside a low priority tree without increasing the priority of the entire tree.To address this, this PR introduces a separate priority field called
pendingUpdatePriority(not 100% sold on the name). The new field represents the priority of the pending props and state, which may be lower than the priority of the entire tree. Now, when the scheduler is searching for work to perform, it only matches if thependingUpdatePrioritymatches. It continues to use thependingWorkPriorityas a fast hint to skip subtrees if they do not match. (To elaborate further: because the work priority is the highest priority of any node in the entire tree, if it does not match, there's no point in searching any deeper.)Based on #7344. Compare view: acdlite/react@fibersetstate...fiberupdatepriority
@sebmarkbage