Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make time-slicing opt-in #21072

Merged
merged 11 commits into from
Apr 9, 2021
Merged

Conversation

rickhanlonii
Copy link
Member

@rickhanlonii rickhanlonii commented Mar 24, 2021

Overview

This PR adds a new flag to synchronously flush updates in a task by default.

This has the practical effect of making time-slicing optional in the next major version.

Motivation

As we've iterated on concurrent features like Suspense and startTransition, we've seen the benefits of Time-slicing not only in those APIs, but also for time-slicing updates by default as shown in the talk above. Time slicing allows React to yield to other work while rendering and allow higher priority tasks such as user interactions, network events, or other renders to be processed. Based on this research, time-slicing by default is the future of React.

However, time-slicing can cause issues with existing code that was not written with yielding in mind. For example, by yielding to other work, we allow for the possibility that external sources of information read during render are updated. This can cause tearing - where part of the UI displays an old value and another part shows a new value. This isn't a React specific bug - it's the logical consequence of concurrency.

We've introduced warnings and dev-behavior to StrictMode to help users catch these types of issues, however there is still a large part of the community that is not ready for the new behavior by default. If we were to keep time-slicing on by default for all updates, then most users would need to convert their entire app to support concurrency all at once before upgrading to the next major version of React.

Changes

This PR updates the default behavior of React in the next major version to flush updates synchronously in a task, similar to how the current behavior works. This means users will not use time-slicing by default when upgrading to the concurrent root, and their app will continue to synchronously render (almost) the same as before.

To opt-in to time-slicing, users can incrementally adopt concurrent features such are Suspense (which will concurrently re-try rendering with time-slicing) or startTransition (which will render updates concurrently with time-slicing). This strategy allows users to begin to use concurrent features in subtrees of their app, while working to adopt StrictMode globally and prepare for the future of React with time-slicing by default.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 24, 2021
@sizebot
Copy link

sizebot commented Mar 24, 2021

Comparing: 8a4a59c...7e9bb7b

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js +0.11% 122.55 kB 122.69 kB +0.11% 39.34 kB 39.39 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.10% 129.12 kB 129.25 kB +0.11% 41.43 kB 41.47 kB
facebook-www/ReactDOM-prod.classic.js +0.13% 411.67 kB 412.22 kB +0.10% 76.17 kB 76.25 kB
facebook-www/ReactDOM-prod.modern.js +0.14% 399.73 kB 400.27 kB +0.11% 74.26 kB 74.34 kB
facebook-www/ReactDOMForked-prod.classic.js +0.13% 411.67 kB 412.22 kB +0.10% 76.18 kB 76.25 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactIs-dev.classic.js +0.75% 9.99 kB 10.07 kB +0.41% 2.71 kB 2.72 kB
facebook-www/ReactIs-dev.modern.js +0.73% 10.26 kB 10.33 kB +0.40% 2.76 kB 2.77 kB
facebook-www/SchedulerTracing-dev.modern.js +0.66% 11.28 kB 11.36 kB +0.48% 2.49 kB 2.50 kB
facebook-www/SchedulerTracing-dev.classic.js +0.66% 11.29 kB 11.36 kB +0.48% 2.49 kB 2.50 kB

Generated by 🚫 dangerJS against 7e9bb7b

@rickhanlonii rickhanlonii force-pushed the rh-default-sync branch 8 times, most recently from 405a9af to 73cde84 Compare April 6, 2021 19:52
@rickhanlonii
Copy link
Member Author

done

@rickhanlonii rickhanlonii changed the title [WIP] Add enableSyncDefaultUpdates Flush default updates synchronously in a task Apr 6, 2021
@rickhanlonii rickhanlonii marked this pull request as ready for review April 6, 2021 20:50
@rickhanlonii rickhanlonii changed the title Flush default updates synchronously in a task Flush updates synchronously by default Apr 6, 2021
@rickhanlonii rickhanlonii changed the title Flush updates synchronously by default Make time-slicing opt-in Apr 6, 2021
@gaearon gaearon mentioned this pull request Apr 7, 2021
63 tasks
});
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.unstable_startTransition(() => {
// TODO: Batched updates need to be inside startTransition?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going on here? Seems like a bug

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not taking the executionContext into account when scheduling the sync updates, so the updates inside of batchedUpdates are each flushed synchronously

This would fix it in ensureRootIsScheduled, but I don't know enough about batchedUpdates or executionContext to know if this is the desired behavior/fix:

Screen Shot 2021-04-09 at 7 10 58 PM

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's follow up with this, I want to merge so that I don't get another merge conflict since this is so big.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nvm, I think this is fine. batchedUpdates doesn't change the priority, so it makes sense you need to wrap these with startTransition when the sync-by-default flag is on.

I had misinterpreted the TODO as saying that without startTransition, the updates wouldn't be batched.

Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay!

@rickhanlonii rickhanlonii merged commit 933880b into facebook:master Apr 9, 2021
@rickhanlonii rickhanlonii deleted the rh-default-sync branch April 9, 2021 23:50
duuliy added a commit to duuliy/react that referenced this pull request Apr 12, 2021
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Apr 20, 2021
Summary:
This sync includes the following changes:
- **[f7cdc8936](facebook/react@f7cdc8936 )**: Also turn off enableSyncDefaultUpdates in RN test renderer ([#21293](facebook/react#21293)) //<Ricky>//
- **[4c9eb2af1](facebook/react@4c9eb2af1 )**: Add dynamic flags to React Native ([#21291](facebook/react#21291)) //<Ricky>//
- **[9eddfbf5a](facebook/react@9eddfbf5a )**: [Fizz] Two More Fixes ([#21288](facebook/react#21288)) //<Sebastian Markbåge>//
- **[11b07597e](facebook/react@11b07597e )**: Fix classes ([#21283](facebook/react#21283)) //<Sebastian Markbåge>//
- **[96d00b9bb](facebook/react@96d00b9bb )**: [Fizz] Random Fixes ([#21277](facebook/react#21277)) //<Sebastian Markbåge>//
- **[81ef53953](facebook/react@81ef53953 )**: Always insert a dummy node with an ID into fallbacks ([#21272](facebook/react#21272)) //<Sebastian Markbåge>//
- **[a4a940d7a](facebook/react@a4a940d7a )**: [Fizz] Add unsupported Portal/Scope components ([#21261](facebook/react#21261)) //<Sebastian Markbåge>//
- **[f4d7a0f1e](facebook/react@f4d7a0f1e )**: Implement useOpaqueIdentifier ([#21260](facebook/react#21260)) //<Sebastian Markbåge>//
- **[dde875dfb](facebook/react@dde875dfb )**: [Fizz] Implement Hooks ([#21257](facebook/react#21257)) //<Sebastian Markbåge>//
- **[a597c2f5d](facebook/react@a597c2f5d )**: [Fizz] Fix reentrancy bug ([#21270](facebook/react#21270)) //<Sebastian Markbåge>//
- **[15e779d92](facebook/react@15e779d92 )**: Reconciler should inject its own version into DevTools hook ([#21268](facebook/react#21268)) //<Brian Vaughn>//
- **[4f76a28c9](facebook/react@4f76a28c9 )**: [Fizz] Implement New Context ([#21255](facebook/react#21255)) //<Sebastian Markbåge>//
- **[82ef450e0](facebook/react@82ef450e0 )**: remove obsolete SharedArrayBuffer ESLint config ([#21259](facebook/react#21259)) //<Henry Q. Dineen>//
- **[dbadfa2c3](facebook/react@dbadfa2c3 )**: [Fizz] Classes Follow Up ([#21253](facebook/react#21253)) //<Sebastian Markbåge>//
- **[686b635b7](facebook/react@686b635b7 )**: Prevent reading canonical property of null ([#21242](facebook/react#21242)) //<Joshua Gross>//
- **[bb88ce95a](facebook/react@bb88ce95a )**: Bugfix: Don't rely on `finishedLanes` for passive effects ([#21233](facebook/react#21233)) //<Andrew Clark>//
- **[343710c92](facebook/react@343710c92 )**: [Fizz] Fragments and Iterable support ([#21228](facebook/react#21228)) //<Sebastian Markbåge>//
- **[933880b45](facebook/react@933880b45 )**: Make time-slicing opt-in ([#21072](facebook/react#21072)) //<Ricky>//
- **[b0407b55f](facebook/react@b0407b55f )**: Support more empty types ([#21225](facebook/react#21225)) //<Sebastian Markbåge>//
- **[39713716a](facebook/react@39713716a )**: Merge isObject branches ([#21226](facebook/react#21226)) //<Sebastian Markbåge>//
- **[8a4a59c72](facebook/react@8a4a59c72 )**: Remove textarea special case from child fiber ([#21222](facebook/react#21222)) //<Sebastian Markbåge>//
- **[dc108b0f5](facebook/react@dc108b0f5 )**: Track which fibers scheduled the current render work ([#15658](facebook/react#15658)) //<Brian Vaughn>//
- **[6ea749170](facebook/react@6ea749170 )**: Fix typo in comment ([#21214](facebook/react#21214)) //<inokawa>//
- **[b38ac13f9](facebook/react@b38ac13f9 )**: DevTools: Add post-commit hook ([#21183](facebook/react#21183)) //<Brian Vaughn>//
- **[b943aeba8](facebook/react@b943aeba8 )**: Fix: Passive effect updates are never sync ([#21215](facebook/react#21215)) //<Andrew Clark>//
- **[d389c54d1](facebook/react@d389c54d1 )**: Offscreen: Use JS stack to track hidden/unhidden subtree state ([#21211](facebook/react#21211)) //<Brian Vaughn>//
- **[c486dc1a4](facebook/react@c486dc1a4 )**: Remove unnecessary processUpdateQueue ([#21199](facebook/react#21199)) //<Sebastian Markbåge>//
- **[cf45a623a](facebook/react@cf45a623a )**: [Fizz] Implement Classes ([#21200](facebook/react#21200)) //<Sebastian Markbåge>//
- **[75c616554](facebook/react@75c616554 )**: Include actual type of `Profiler#id` on type mismatch ([#20306](facebook/react#20306)) //<Sebastian Silbermann>//
- **[1214b302e](facebook/react@1214b302e )**: test: Fix "couldn't locate all inline snapshots" ([#21205](facebook/react#21205)) //<Sebastian Silbermann>//
- **[1a02d2792](facebook/react@1a02d2792 )**: style: delete unused isHost check ([#21203](facebook/react#21203)) //<wangao>//
- **[782f689ca](facebook/react@782f689ca )**: Don't double invoke getDerivedStateFromProps for module pattern ([#21193](facebook/react#21193)) //<Sebastian Markbåge>//
- **[e90c76a65](facebook/react@e90c76a65 )**: Revert "Offscreen: Use JS stack to track hidden/unhidden subtree state" ([#21194](facebook/react#21194)) //<Brian Vaughn>//
- **[1f8583de8](facebook/react@1f8583de8 )**: Offscreen: Use JS stack to track hidden/unhidden subtree state ([#21192](facebook/react#21192)) //<Brian Vaughn>//
- **[ad6e6ec7b](facebook/react@ad6e6ec7b )**: [Fizz] Prepare Recursive Loop for More Types ([#21186](facebook/react#21186)) //<Sebastian Markbåge>//
- **[172e89b4b](facebook/react@172e89b4b )**: Reland Remove redundant initial of isArray ([#21188](facebook/react#21188)) //<Sebastian Markbåge>//
- **[7c1ba2b57](facebook/react@7c1ba2b57 )**: Proposed new Suspense layout effect semantics ([#21079](facebook/react#21079)) //<Brian Vaughn>//
- **[316aa3686](facebook/react@316aa3686 )**: [Scheduler] Fix de-opt caused by out-of-bounds access ([#21147](facebook/react#21147)) //<Andrey Marchenko>//
- **[b4f119cdf](facebook/react@b4f119cdf )**: Revert "Remove redundant initial of isArray ([#21163](facebook/react#21163))" //<Sebastian Markbage>//
- **[c03197063](facebook/react@c03197063 )**: Revert "apply prettier ([#21165](facebook/react#21165))" //<Sebastian Markbage>//
- **[94fd1214d](facebook/react@94fd1214d )**: apply prettier ([#21165](facebook/react#21165)) //<Behnam Mohammadi>//
- **[b130a0f5c](facebook/react@b130a0f5c )**: Remove redundant initial of isArray ([#21163](facebook/react#21163)) //<Behnam Mohammadi>//
- **[2c9fef32d](facebook/react@2c9fef32d )**: Remove redundant initial of hasOwnProperty ([#21134](facebook/react#21134)) //<Behnam Mohammadi>//
- **[1cf9978d8](facebook/react@1cf9978d8 )**: Don't pass internals to callbacks ([#21161](facebook/react#21161)) //<Sebastian Markbåge>//
- **[b9e4c10e9](facebook/react@b9e4c10e9 )**: [Fizz] Implement all the DOM attributes and special cases ([#21153](facebook/react#21153)) //<Sebastian Markbåge>//
- **[f8ef4ff57](facebook/react@f8ef4ff57 )**: Flush discrete passive effects before paint ([#21150](facebook/react#21150)) //<Andrew Clark>//
- **[b48b38af6](facebook/react@b48b38af6 )**: Support nesting of startTransition and flushSync (alt) ([#21149](facebook/react#21149)) //<Sebastian Markbåge>//

Changelog:
[General][Changed] - React Native sync for revisions c9aab1c...f7cdc89

jest_e2e[run_all_tests]

Reviewed By: rickhanlonii

Differential Revision: D27740113

fbshipit-source-id: 6e27204d78e3e16ed205170006cb97c0d6bfa957
acdlite added a commit to acdlite/react that referenced this pull request Apr 20, 2021
Instead of `performSyncWorkOnRoot`.

The conceptual model is that the only difference between sync default
updates (in React 18) and concurrent default updates (in a future major
release) is time slicing. All other behavior should be the same
(i.e. the stuff in `finishConcurrentRender`).

Given this, I think it makes more sense to model the implementation this
way, too. This exposed a quirk in the previous implementation where
non-sync work was sometimes mistaken for sync work and flushed too
early. In the new implementation, `performSyncWorkOnRoot` is only used
for truly synchronous renders (i.e. `SyncLane`), which should make these
mistakes less common.

Fixes most of the tests marked with TODOs from facebook#21072.
acdlite added a commit that referenced this pull request Apr 21, 2021
Instead of `performSyncWorkOnRoot`.

The conceptual model is that the only difference between sync default
updates (in React 18) and concurrent default updates (in a future major
release) is time slicing. All other behavior should be the same
(i.e. the stuff in `finishConcurrentRender`).

Given this, I think it makes more sense to model the implementation this
way, too. This exposed a quirk in the previous implementation where
non-sync work was sometimes mistaken for sync work and flushed too
early. In the new implementation, `performSyncWorkOnRoot` is only used
for truly synchronous renders (i.e. `SyncLane`), which should make these
mistakes less common.

Fixes most of the tests marked with TODOs from #21072.
koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
* Add enableSyncDefaultUpdates feature flag

* Add enableSyncDefaultUpdates implementation

* Fix tests

* Switch feature flag to true by default

* Finish concurrent render whenever for non-sync lanes

* Also return DefaultLane with eventLane

* Gate interruption test

* Add continuout native event test

* Fix tests from rebasing main

* Hardcode lanes, remove added export

* Sync forks
koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
Instead of `performSyncWorkOnRoot`.

The conceptual model is that the only difference between sync default
updates (in React 18) and concurrent default updates (in a future major
release) is time slicing. All other behavior should be the same
(i.e. the stuff in `finishConcurrentRender`).

Given this, I think it makes more sense to model the implementation this
way, too. This exposed a quirk in the previous implementation where
non-sync work was sometimes mistaken for sync work and flushed too
early. In the new implementation, `performSyncWorkOnRoot` is only used
for truly synchronous renders (i.e. `SyncLane`), which should make these
mistakes less common.

Fixes most of the tests marked with TODOs from facebook#21072.
@gaearon gaearon mentioned this pull request Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants