Don't bind expiration time to render callback #16715
Closed
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
A FiberRoot can have pending work at many distinct priorities. (Note: we refer to these levels as "expiration times" to distinguish the concept from Scheduler's notion of priority levels, which represent broad categories of work. React expiration times are more granualar. They're more like a concurrent thread ID, which also happens to correspond to a moment on a timeline. It's an overloaded concept and I'm handwaving over some of the details.) Given a root, there's no convenient way to read all the pending levels in the entire tree, i.e. there's no single queue-like structure that tracks all the levels, because that granularity of information is not needed by our algorithms. Instead we track the subset of information that we actually need — most importantly, the highest priority level that exists in the entire tree. Aside from that, the other information we track includes the range of pending levels that are known to be suspended, and therefore should not be worked on. This is a refactor of how that information is tracked, and what each field represents: - A *pending* level is work that is unfinished, or not yet committed. This includes work that is suspended from committing. `firstPendingTime` and `lastPendingTime` represent the range of pending work. (Previously, "pending" was the same as "not suspended.") - A *suspended* level is work that did not complete because data was missing. `firstSuspendedTime` and `lastSuspendedTime` represent the range of suspended work. It is a subset of the pending range. (These fields are new to this commit.) - `nextAfterSuspendedTime` represents the next known level that comes after the suspended range. This commit doesn't change much in terms of observable behavior. The one change is that, when a level is suspended, React will continue working on the next known level instead of jumping straight to the last pending level. Subsequent commits will use this new structure for a more substantial refactor for how tasks are scheduled per root.
Given a FiberRoot, we should be able to determine the next expiration time that needs to be worked on, taking into account the levels that are pending, suspended, pinged, and so on. This removes the `expirationTime` argument from `scheduleCallbackForRoot`, and renames it to `ensureRootIsScheduled` to reflect the new signature. The expiration time is instead read from the root using a new function, `getNextExpirationTimeToWorkOn`. The next step will be to remove the `expirationTime` argument from `renderRoot`, too.
This is a fragile pattern because there's only meant to be a single task per root, running at a single expiration time. Instead of binding the expiration time to the render task, or closing over it, we should determine the correct expiration time to work on using fields we store on the root object itself. This removes the "return a continuation" pattern from the `renderRoot` function. Continuation handling is now handled by the wrapper function, which I've renamed from `runRootCallback` to `performWorkOnRoot`. That function is merely an entry point to `renderRoot`, so I've also removed the callback argument. So to sum up, at at the beginning of each task, `performWorkOnRoot` determines which expiration time to work on, then calls `renderRoot`. And before exiting, it checks if it needs to schedule another task.
acdlite
commented
Sep 9, 2019
| @@ -494,7 +494,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { | |||
| return <Text text="(empty)" />; | |||
| } | |||
| return ( | |||
| <Suspense> | |||
| <Suspense fallback="Loading..."> | |||
There was a problem hiding this comment.
This test was testing the wrong thing, because a Suspense without a fallback will result in an error. So what this was actually testing was the "on error, try again at the lowest pending level."
acdlite
commented
Sep 9, 2019
packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js
Show resolved
Hide resolved
|
ReactDOM: size: 0.0%, gzip: 0.0% Details of bundled changes.Comparing: 9ce8711...09461dc react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
|
acdlite
force-pushed
the
dont-bind-expiration-time
branch
3 times, most recently
from
a67ce79 to
ec600a9
Compare
It's no longer used anywhere
acdlite
force-pushed
the
dont-bind-expiration-time
branch
from
ec600a9 to
9a54113
Compare
acdlite
commented
Sep 10, 2019
| @@ -422,7 +422,10 @@ exports[`ReactDebugFiberPerf warns if an in-progress update is interrupted 1`] = | |||
| `; | |||
|
|
|||
| exports[`ReactDebugFiberPerf warns if async work expires (starvation) 1`] = ` | |||
| "⚛ (Committing Changes) | |||
| "⚛ (React Tree Reconciliation: Completed Root) | |||
| ⚛ Foo [mount] | |||
There was a problem hiding this comment.
This was actually wrong before, because it improperly dropped the measurements from the render phase.
Jessidhia
reviewed
Sep 10, 2019
| if (expirationTime !== Sync) { | ||
| // Since we know we're in a React event, we can clear the current | ||
| // event time. The next update will compute a new event time. | ||
| currentEventTime = NoWork; |
There was a problem hiding this comment.
Is this a React event?
(is any queued async update considered an "event"?)
There was a problem hiding this comment.
The "event" here refers to a scheduled callback with the scheduler. I.e. clean stack.
If the work-in-progress root already suspended with a delay, then the current render definitely won't finish. We should interrupt the render and switch to the incoming update.
acdlite
force-pushed
the
dont-bind-expiration-time
branch
4 times, most recently
from
cc83dca to
0e92ca1
Compare
sebmarkbage
reviewed
Sep 10, 2019
| // try restarting now instead. | ||
| root.lastPingedTime = expirationTime; | ||
| prepareFreshStack(root, expirationTime); | ||
| return; |
There was a problem hiding this comment.
If lastPingedTime === expirationTime then I wouldn't expect there to be a callback already scheduled, right? So how does this pick up work back up?
There was a problem hiding this comment.
sebmarkbage
reviewed
Sep 10, 2019
| const nextTime = getNextRootExpirationTimeToWorkOn(root); | ||
| if (nextTime !== NoWork && nextTime !== expirationTime) { | ||
| // There's additional work on this root. | ||
| return; |
There was a problem hiding this comment.
It doesn't really hurt to schedule a callback that is just going to get canceled, right? Maybe it's ok to let it schedule a callback just in case we don't get called for a while to other work, we might as well commit?
There was a problem hiding this comment.
sebmarkbage
reviewed
Sep 10, 2019
| // We should prefer to render the fallback of at the last suspended | ||
| // level. Ping the last suspended level to try rendering it again. | ||
| root.lastPingedTime = lastSuspendedTime; | ||
| return; |
There was a problem hiding this comment.
I'm paranoid about these not having a callback scheduled and us stalling.
There was a problem hiding this comment.
I put ensureRootIsScheduled in the finally block of performWorkOnRoot to try to account for this.
I think it could theoretically happen for some of the early flush paths, which bypass performWorkOnRoot. But since they bypass performWorkOnRoot, they also won’t cause an existing callback to be canceled.
Similar to the previous commit, if we suspend with a delay, and something in the return path has a pending update, we should abort the current render and switch to the update instead.
acdlite
force-pushed
the
dont-bind-expiration-time
branch
from
0e92ca1 to
2d93c7e
Compare
Instead of backtracking the return path. The main advantage over the backtracking approach is that we don't have to backtrack from the source fiber. (The main disadvantages are that it requires another module-level variable, and that it could include updates from unrelated sibling paths.)
|
@sebmarkbage Regarding your paranoia that we'll neglect to schedule a callback, I opened a follow-up PR that tries to prevent a refactor hazard: #16743 |
sebmarkbage
approved these changes
Sep 11, 2019
|
Closed by ab4951f |
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.
Based on #16663 and #16678
This is a fragile pattern because there's only meant to be a single task per root, running at a single expiration time. Instead of binding the expiration time to the render task, or closing over it, we should determine the correct expiration time to work on using fields we store on the root object itself.
This removes the "return a continuation" pattern from the
renderRootfunction. Continuation handling is now handled by the wrapper function, which I've renamed fromrunRootCallbacktoperformWorkOnRoot. That function is merely an entry point torenderRoot, so I've also removed the callback argument.So to sum up, at at the beginning of each task,
performWorkOnRootdetermines which expiration time to work on, then callsrenderRoot. And before exiting, it checks if it needs to schedule another task.