Add dedicated LanePriority for Suspense retries #19287
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
A "retry" is a special type of update that attempts to flip a Suspense boundary from its placeholder state back to its primary/resolved state. Currently, retries are given default priority, using the same algorithm as normal updates and occupying range of lanes. This adds a new range of lanes dedicated specifically to retries, and gives them lower priority than normal updates. A couple of existing tests were affected because retries are no longer batched with normal updates; they commit in separate batches. Not totally satisfied with this design, but I think things will snap more into place once the rest of the Lanes changes (like how we handle parallel Suspense transitions) have settled.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 723cfc8:
|
Details of bundled changes.Comparing: 40cddfe...723cfc8 react-dom
react-art
react-test-renderer
react-native-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: 🔺+0.1% Size changes (stable) |
Details of bundled changes.Comparing: 40cddfe...723cfc8 react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: 🔺+0.1% Size changes (experimental) |
facebook-github-bot
added
CLA Signed
React Core Team
rickhanlonii
approved these changes
Jul 8, 2020
packages/react-reconciler/src/__tests__/ReactSuspenseCallback-test.internal.js
Show resolved
Hide resolved
packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
Outdated
Show resolved
Hide resolved
| // lanes, leading to the extra intermediate render. This is because when | ||
| // we schedule the fourth update, we're already in the middle of rendering | ||
| // the three others. Since three are only three lanes in the default | ||
| // range, the fourth lane is shifted to slightly lower priority. This |
There was a problem hiding this comment.
the fourth lane is shifted to slightly lower priority
Could you expand in this more? I would think that the "priority" doesn't change, the work just gets pushed out further?
There was a problem hiding this comment.
The "lane priority" of a bit is determined by which range of bits it belongs to. We can't assign a new update to a lane that's already in the middle of rendering. In this (fairly contrived) test scenario, we ran out of lanes in the Default range so we had to pick from the next available lane in a lower priority range. Another option would be to throw out the in-progress work and start over — then we're free to pick any lane we want. tl;dr this is a batching heuristic that doesn't matter that much in most scenarios and probably isn't the last time this will change. It's only observable in tests if you do clever stuff like partial rendering, which we don't officially support and is only used in our own test suite.
There was a problem hiding this comment.
I think what's confusing to me in "the fourth lane is shifted to slightly lower priority" is that the lane isn't what changes priorities? Do you mean "the fourth update is shifted to a (lane in a) slightly lower priority"?
There was a problem hiding this comment.
Oh right. Yeah I meant fourth update :D
| @@ -3923,7 +3923,6 @@ describe('Profiler', () => { | |||
|
|
|||
| expect(onPostCommit).toHaveBeenCalledTimes(1); | |||
| expect(onPostCommit.mock.calls[0][4]).toMatchInteractions([ | |||
| initialRenderInteraction, | |||
There was a problem hiding this comment.
tl;dr the initial render is no longer entangled with the Suspense retry. See rationale from previous comment.
acdlite
force-pushed
the
retry-lane-priority
branch
from
ec9f3d7 to
723cfc8
Compare
|
Note to whoever does this next sync: this is moderately risky. If I weren't possessed of such a daring personality, I might put it behind a feature flag. |
15 tasks
bvaughn
reviewed
Jul 9, 2020
| // from its placeholder state to its primary/resolved state. | ||
| let lane = findLane(RetryRangeStart, RetryRangeEnd, wipLanes); | ||
| if (lane === NoLane) { | ||
| lane = pickArbitraryLane(RetryLanes); |
There was a problem hiding this comment.
Late to the party here (sorry, oncall this week and a bit behind) but catching up now, I had a small thought.
The naming of pickArbitraryLane suggests (to me) that it could return a different value each time it's called. Its current implementation always happens to return a predictable/consistent value, but do we think this might ever change? If it did, the usage of it would be at conflict with the above comment:
This should be pure function, so that it always returns the same lane for given inputs.
...in which case maybe calling getLowestPriorityLane directly would be "better"
There was a problem hiding this comment.
“arbitrary” in this case means it doesn’t matter if it’s the lowest priority lane or highest priority lane or anything in between.
For example, it communicates if there’s a refactor where the most significant lane is available for some other reason, you could use that instead without affecting the algorithm.
2 tasks
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Jul 10, 2020
Now that Suspense retries have their own dedicated set of lanes (facebook#19287), we can determine if a render includes only retries by checking if its lanes are a subset of the retry lanes. Previously we inferred this by checking `workInProgressRootLatestProcessedEventTime`. If it's not set, that implies that no updates were processed in the current render, which implies it must be a Suspense retry. The eventual plan is to get rid of `workInProgressRootLatestProcessedEventTime` and instead track event times on the root; this change is one the steps toward that goal. The relevant tests were originally added in facebook#15769.
acdlite
added a commit
that referenced
this pull request
Jul 10, 2020
Now that Suspense retries have their own dedicated set of lanes (#19287), we can determine if a render includes only retries by checking if its lanes are a subset of the retry lanes. Previously we inferred this by checking `workInProgressRootLatestProcessedEventTime`. If it's not set, that implies that no updates were processed in the current render, which implies it must be a Suspense retry. The eventual plan is to get rid of `workInProgressRootLatestProcessedEventTime` and instead track event times on the root; this change is one the steps toward that goal. The relevant tests were originally added in #15769.
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.
A "retry" is a special type of update that attempts to flip a Suspense boundary from its placeholder state back to its primary/resolved state.
Currently, retries are given default priority, using the same algorithm as normal updates and occupying range of lanes.
This adds a new range of lanes dedicated specifically to retries, and gives them lower priority than normal updates.
A couple of existing tests were affected because retries are no longer batched with normal updates; they commit in separate batches.
Not totally satisfied with this design, but I think things will snap more into place once the rest of the Lanes changes (like how we handle parallel Suspense transitions) have settled.