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.

Sign up for GitHub

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

Jump to bottom

When a root expires, flush all expired work in a single batch #13503

Merged
merged 1 commit into from
Sep 6, 2018
Merged

When a root expires, flush all expired work in a single batch #13503

merged 1 commit into from
Sep 6, 2018

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Aug 28, 2018

Still need to add some Suspense tests, and some comments. Pushing so Dan can test if it fixes the issue in the fixture.

@acdlite acdlite requested a review from gaearon August 28, 2018 20:04
@pull-bot
Copy link

pull-bot commented Aug 28, 2018 •
edited
Loading

ReactDOM: size: 🔺+0.2%, gzip: 🔺+0.1%

Details of bundled changes.

Comparing: bb62722...077addd

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.2% +0.2% 644.92 KB 645.9 KB 151.12 KB 151.36 KB UMD_DEV
react-dom.production.min.js 🔺+0.2% 🔺+0.1% 92.16 KB 92.31 KB 30.08 KB 30.1 KB UMD_PROD
react-dom.development.js +0.2% +0.2% 640.27 KB 641.25 KB 149.71 KB 149.96 KB NODE_DEV
react-dom.production.min.js 🔺+0.2% 🔺+0.1% 92.1 KB 92.25 KB 29.67 KB 29.71 KB NODE_PROD
ReactDOM-dev.js +0.1% +0.2% 662.6 KB 663.58 KB 152.06 KB 152.31 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.1% 🔺+0.1% 286.89 KB 287.29 KB 53.15 KB 53.2 KB FB_WWW_PROD
react-dom.profiling.min.js +0.2% +0.2% 95.05 KB 95.2 KB 30.31 KB 30.36 KB NODE_PROFILING
ReactDOM-profiling.js +0.1% +0.1% 293.27 KB 293.67 KB 54.48 KB 54.53 KB FB_WWW_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.2% +0.3% 438.59 KB 439.57 KB 98.24 KB 98.5 KB UMD_DEV
react-art.production.min.js 🔺+0.2% 🔺+0.1% 84.57 KB 84.72 KB 25.98 KB 26.01 KB UMD_PROD
react-art.development.js +0.3% +0.3% 370.34 KB 371.32 KB 81.14 KB 81.4 KB NODE_DEV
react-art.production.min.js 🔺+0.3% 🔺+0.3% 49.54 KB 49.68 KB 15.26 KB 15.31 KB NODE_PROD
ReactART-dev.js +0.3% +0.3% 375.29 KB 376.27 KB 79.93 KB 80.17 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.3% 🔺+0.2% 159.84 KB 160.24 KB 27.11 KB 27.18 KB FB_WWW_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.3% +0.3% 382.37 KB 383.34 KB 83.72 KB 83.97 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.3% 🔺+0.3% 50.75 KB 50.89 KB 15.51 KB 15.56 KB UMD_PROD
react-test-renderer.development.js +0.3% +0.3% 377.95 KB 378.93 KB 82.6 KB 82.85 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.3% 🔺+0.4% 50.46 KB 50.6 KB 15.35 KB 15.4 KB NODE_PROD
ReactTestRenderer-dev.js +0.3% +0.3% 383.07 KB 384.05 KB 81.64 KB 81.89 KB FB_WWW_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.3% +0.3% 366.23 KB 367.21 KB 79.27 KB 79.51 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.3% 🔺+0.3% 49.1 KB 49.25 KB 14.76 KB 14.79 KB NODE_PROD
react-reconciler-persistent.development.js +0.3% +0.3% 364.8 KB 365.78 KB 78.69 KB 78.94 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+0.3% 🔺+0.3% 49.11 KB 49.26 KB 14.76 KB 14.8 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.2% +0.2% 497.65 KB 498.63 KB 110.11 KB 110.37 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.2% 🔺+0.1% 214.75 KB 215.15 KB 37.45 KB 37.5 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.2% +0.2% 497.38 KB 498.36 KB 110.04 KB 110.3 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 🔺+0.2% 🔺+0.2% 204.57 KB 204.97 KB 35.8 KB 35.86 KB RN_OSS_PROD
ReactFabric-dev.js +0.2% +0.2% 487.83 KB 488.81 KB 107.69 KB 107.96 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.2% 🔺+0.2% 197.07 KB 197.47 KB 34.31 KB 34.36 KB RN_FB_PROD
ReactFabric-dev.js +0.2% +0.2% 487.87 KB 488.85 KB 107.71 KB 107.97 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.2% 🔺+0.2% 197.1 KB 197.5 KB 34.32 KB 34.37 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.2% +0.1% 211.68 KB 212.08 KB 37.14 KB 37.2 KB RN_OSS_PROFILING
ReactFabric-profiling.js +0.2% +0.2% 203.01 KB 203.41 KB 35.59 KB 35.65 KB RN_OSS_PROFILING
ReactNativeRenderer-profiling.js +0.2% +0.1% 220.16 KB 220.56 KB 38.63 KB 38.68 KB RN_FB_PROFILING
ReactFabric-profiling.js +0.2% +0.2% 202.97 KB 203.37 KB 35.57 KB 35.63 KB RN_FB_PROFILING

schedule

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
schedule.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
schedule.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

@gaearon
Copy link
Collaborator

gaearon commented Aug 28, 2018

It looks like it's helping. Still gets stuck sometimes but I suppose it's just two expirations happening one after the other (because the commit itself took a long time). But nothing like it was getting stuck before.

@acdlite acdlite force-pushed the batchexpiredupdates branch 2 times, most recently from f28c369 to 801a2d8 Compare September 5, 2018 17:20
@acdlite
Copy link
Collaborator Author

acdlite commented Sep 5, 2018

@gaearon Ready for review

@acdlite acdlite force-pushed the batchexpiredupdates branch 3 times, most recently from 7f19945 to 0d9802f Compare September 5, 2018 17:38
@acdlite
When a root expires, flush all expired work in a single batch
077addd 
Instead of flushing each level one at a time.
@gaearon gaearon mentioned this pull request Sep 5, 2018
Merged
gaearon
gaearon approved these changes Sep 5, 2018
View reviewed changes
Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

This makes sense to me by itself but doesn't actually help the issue reproducible in the fixture. If you try to type a lot and then constantly add and remove characters it will eventually expire with isExpired = false. However the scheduler thinks there's still some time before the timeout, so didTimeout is false. I'm not sure why but we need to fix this too?

@gaearon gaearon merged commit f765f02 into facebook:master Sep 6, 2018
@gaearon
Copy link
Collaborator

gaearon commented Sep 6, 2018

Let's get this fix in, but we'll need a follow-up for scheduler itself.

This was referenced Sep 7, 2018
Closed
Closed
Open
Closed
Open
Open
Open
Open
Open
Merged
Closed
Closed
Merged
@dependencies dependencies bot mentioned this pull request Oct 1, 2018
Open
@gaearon gaearon mentioned this pull request Oct 13, 2018
Closed
This was referenced Nov 1, 2018
Closed
Closed
This was referenced Dec 1, 2018
Open
Open
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
@acdlite @jetoneza
When a root expires, flush all expired work in a single batch (facebo…
d343e8f 
…ok#13503)

Instead of flushing each level one at a time.
This was referenced Sep 5, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mihalskiy mihalskiy mihalskiy approved these changes

@gaearon gaearon gaearon approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@acdlite @pull-bot @gaearon @mihalskiy @facebook-github-bot