[Fiber] Refactor bailoutOnFinishedWork #8613
Conversation
sebmarkbage
changed the title
| function updateHostComponent(current, workInProgress) { | ||
| const nextProps = workInProgress.pendingProps; | ||
| pushHostContext(workInProgress); |
There was a problem hiding this comment.
This wasn't always pushed before such as when props.hidden. Now it should always push.
|
I'm still looking at bailoutOnLowPriority which also has inconsistent pushes. I can do that in a follow up. Want to see if we can make it avoid calling completeWork in that case. |
| // TODO: This call is burried a bit too deep. It would be nice to have | ||
| // a single point which happens right before any new work and | ||
| // unfortunately this is it. | ||
| resetContextStack(); |
There was a problem hiding this comment.
Could we put the reassignment of priorityContext (line 208) inside resetContextStack? It's not quite the same as the other contexts but it serves a similar purpose.
There was a problem hiding this comment.
We could but I suspect that with #8611 these will be unified to a single call to an external module which is why I grouped them inside of just inlining them.
There was a problem hiding this comment.
That is correct. At least in its current form, #8611 unifies these context stacks so that it is no longer possible to reset them independently. You can only reset both or unwind both to a current Fiber.
| } | ||
| } else if (nextProps === null || memoizedProps === nextProps || ( | ||
| // TODO: Disable this before release, since it is not part of the public API | ||
| // I use this for testing to compare the relative overhead of classes. |
There was a problem hiding this comment.
Will we actually do this?
| // If the state is the same as before, that's a bailout because we had | ||
| // no work matching this priority. | ||
| return bailoutOnAlreadyFinishedWork(current, workInProgress); | ||
| } | ||
| const element = state.element; | ||
| reconcileChildren(current, workInProgress, element); | ||
| workInProgress.memoizedState = state; |
There was a problem hiding this comment.
Shouldn't we return workInProgress.child inside this branch? I get that bailoutOnAlreadyFinishedWork does that anyway but if there's state then it's not really a bailout, is it?
There was a problem hiding this comment.
Ah yes. Good catch.
| // Return true if the first pending update has greater or equal priority. | ||
| return comparePriority(queue.first.priorityLevel, priorityLevel) <= 0; | ||
| } | ||
| exports.hasPendingUpdate = hasPendingUpdate; |
| } | ||
| const element = state.element; | ||
| reconcileChildren(current, workInProgress, element); | ||
| workInProgress.memoizedState = state; |
There was a problem hiding this comment.
GitHub suppressed my comment on an older commit so here it is again:
Shouldn't we return workInProgress.child inside this branch? I get that bailoutOnAlreadyFinishedWork does that anyway but if there's state then it's not really a bailout, is it?
This consolidates the reset of context to one single place right before any new work is started. This place is burried in a bit of an awkward place but findNextUnitOfWork happens right before any new work starts. That way we're guaranteed to have an empty stack before we start anything new. The benefit of this is that we don't have to rely .return being null when entering beginWork but we also don't have to check it every time in the hot path.
This just moves things around to continue the pattern that we already started.
The functional component branch is a bit special since it has sCU in it. The class component branch actually already covers this in a bit convoluted way. I will next replicate this check in each other branch.
This doesn't need to compare props because it doesn't use props, only state.
Now we only bail out once we've entered each branch. This has some repetition with regard to hasContextChanged but I'm hoping we can just get rid of all those branches at some point since they're mostly for backwards compatibility right now.
This is no longer needed. It is effectively covered by other branches that tries to being merging the update queue which yields the same state object as before if there is no work. That in turn bails out. We can shortcut in the relevant paths if needed.
Since we only call bailout from within the host component branch now, we can just move this to the branch where we already know that this is a host component. As part of this I noticed that we don't always push the host context so I moved that to the beginning of the branch.
This is the same thing as the previous commit but with class, root and portal. I noticed host and portal now already covers this case in their branches since pushHostContainer is always at the top.
sebmarkbage
force-pushed
the
fiberrefactorbailout
branch
from
edd15b7 to
26c82ce
Compare
See individual commits.
This consolidates bailouts on finished finished work into each branch behind the tag.