Remove invokeGuardedCallback from commit phase #21666
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
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 101ea9f...4556567 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
gaearon
commented
Jun 11, 2021
| } catch (error) { | ||
| captureCommitPhaseError(fiber, fiber.return, error); | ||
| } | ||
| setCurrentDebugFiberInDEV(fiber); |
There was a problem hiding this comment.
Note: this is OK to call outside DEV.
|
So I tested this in non-synthetic case just to be sure. Here's results from a real app rendering a (relative small) data table on input change. Before:
After:
It cut down rendering per keystroke from 1.2s to 300ms, so 4x DEV improvement for a common interaction. And a 100x absolute improvement. :) |
sebmarkbage
approved these changes
Jun 14, 2021
koto
pushed a commit
to koto/react
that referenced
this pull request
Jun 15, 2021
* Remove invokeGuardedCallback from commit phase * Sync fork
eps1lon
added a commit
to eps1lon/react
that referenced
this pull request
Jun 22, 2021
This reverts commit a8f5e77.
|
This changed how errors thrown in effects are handled if they were children of an Error Boundary. Previously they were logged to the console and triggered the error overlay in an Error Boundary. With this change the error is no longer logged and triggers no error overlay which is different compared to errors thrown when rendering. #21712 has more context. |
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Jun 22, 2021
Summary: This sync includes the following changes: - **[43f4cc160](facebook/react@43f4cc160 )**: Fix failing test ([#21697](facebook/react#21697)) //<Dan Abramov>// - **[d0f348dc1](facebook/react@d0f348dc1 )**: Fix for failed Suspense layout semantics ([#21694](facebook/react#21694)) //<Brian Vaughn>// - **[bd0a96344](facebook/react@bd0a96344 )**: Throw when `act` is used in production ([#21686](facebook/react#21686)) //<Andrew Clark>// - **[9343f8720](facebook/react@9343f8720 )**: Use the server src files as entry points for the builds/tests ([#21683](facebook/react#21683)) //<Sebastian Markbåge>// - **[502f8a2a0](facebook/react@502f8a2a0 )**: [Fizz/Flight] Don't use default args ([#21681](facebook/react#21681)) //<Sebastian Markbåge>// - **[a8f5e77b9](facebook/react@a8f5e77b9 )**: Remove invokeGuardedCallback from commit phase ([#21666](facebook/react#21666)) //<Dan Abramov>// - **[dbe3363cc](facebook/react@dbe3363cc )**: [Fizz] Implement Legacy renderToString and renderToNodeStream on top of Fizz ([#21276](facebook/react#21276)) //<Sebastian Markbåge>// - **[101ea9f55](facebook/react@101ea9f55 )**: Set deletedTreeCleanUpLevel to 3 ([#21679](facebook/react#21679)) //<Dan Abramov>// - **[1a106bdc2](facebook/react@1a106bdc2 )**: Wrap eventhandle-specific logic in a flag ([#21657](facebook/react#21657)) //<Dan Abramov>// - **[cb30388d1](facebook/react@cb30388d1 )**: Export React Native `AttributeType` Types ([#21661](facebook/react#21661)) //<Timothy Yung>// - **[c1536795c](facebook/react@c1536795c )**: Revert "Make enableSuspenseLayoutEffectSemantics static for www ([#21617](facebook/react#21617))" ([#21656](facebook/react#21656)) //<Sebastian Markbåge>// Changelog: [General][Changed] - React Native sync for revisions c96b78e...568dc35 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D29303157 fbshipit-source-id: 90952885eb2264f4effa04070357b80700bb9be3
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.
Alternative to #21658.
The reason we have this in the first place is to make "pause on uncaught exceptions" work. That's a development feature. However, the way we do this now, especially after the effect traversal refactor, causes quite a bit of a DEV slowdown in commit phase. This makes it harder to have an accurate sense of how slow a render/commit is. While DEV isn't meant to be representative of real perf, it's bad that commits seem disproportionally expensive because of this. It also legitimately slows down the app in development on its own. So we have a devx fix making devx worse. Which is important? I'd say it's not worth it.
Seb proposed that we just remove this and use the prod path in the commit phase. This PR does that.
Example DEV perf difference
This is on a very shallow tree (1 level deep) but with many host children.
Commit before: 10ms
Commit after: 0.1ms
I expect this to also show up in deeper trees, especially the ones with lots of effects etc.