-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: Improve performance of collection updates in React transitions #8204
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
Conversation
</TableBody> | ||
</Table> | ||
</div> | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description of what we should see if it's working vs failing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fast vs slow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I pulled down the PR and reverted the changes in Document. It is definitely painfully slow without the update to Document.
It'd be nice if we could actually test this so we'd know if we ever introduced anything which made it slow again.
I wonder if this can be replicated in a jest test with real timers and if it exceeds some threshold then we fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe. I'd be worried about the timing variability on different machines though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snowystinger Added this in #8209. Should be safe as far as timing is concerned 👍
</TableBody> | ||
</Table> | ||
</div> | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I pulled down the PR and reverted the changes in Document. It is definitely painfully slow without the update to Document.
It'd be nice if we could actually test this so we'd know if we ever introduced anything which made it slow again.
I wonder if this can be replicated in a jest test with real timers and if it exceeds some threshold then we fail.
Fixes #8094
This improves performance of collection updates that occur in React transitions, e.g. startTransition. The performance issue was caused by the following behavior described in the docs for useSyncExternalStore:
Essentially, React would call our
appendChild
to update the fake DOM, we'd trigger theuseSyncExternalStore
subscription, and React would interrupt itself and start over. This would occur for each element that was rendered, resulting in many renders.This behavior seemed unexpected at first, because in general, React's commit phase is blocking. Only the render phase is concurrent. After inspecting the React source code, it turns out that they actually do some DOM updates during the render phase, which can be interrupted. Specifically, they create new DOM elements during the render phase, and then the commit phase is just attaching new subtrees to the existing DOM, and performing updates/removals.
The solution is to avoid triggering collection updates for elements that are not in the document yet. Once React starts the commit phase, it will attach these disconnected nodes and that will result in a single collection update instead of interrupting render.