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

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

Merged
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.

@pull-bot
Copy link

pull-bot commented Aug 28, 2018

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
Instead of flushing each level one at a time.
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants