Skip to content

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Sep 24, 2021

The scheduling profiler markComponentRenderStopped method is supposed to be called when rendering finishes or when a value is thrown (Suspense or Error). Previously we were calling this in a Suspense-only path of throwException:

if (enableSchedulingProfiler) {
markComponentRenderStopped();
markComponentSuspended(sourceFiber, wakeable, rootRenderLanes);
}

This PR updates the code to handle errors (or non-Thenables) thrown as well.

It also moves the mark logic the work loop handleError method, with Suspense/Error agnostic cleanup:

function handleError(root, thrownValue): void {
do {
let erroredWork = workInProgress;
try {

@facebook-github-bot facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Sep 24, 2021
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.

Looks good on reconciler side

@sizebot
Copy link

sizebot commented Sep 24, 2021

Comparing: 1c58cfa...f4ab4e0

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 = 129.54 kB 129.54 kB = 41.29 kB 41.29 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 132.37 kB 132.37 kB = 42.23 kB 42.23 kB
facebook-www/ReactDOM-prod.classic.js = 411.19 kB 411.19 kB = 76.09 kB 76.09 kB
facebook-www/ReactDOM-prod.modern.js = 399.75 kB 399.75 kB = 74.38 kB 74.38 kB
facebook-www/ReactDOMForked-prod.classic.js = 411.19 kB 411.19 kB = 76.09 kB 76.10 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable-semver/react-reconciler/cjs/react-reconciler.profiling.min.js +0.28% 99.30 kB 99.58 kB +0.15% 30.40 kB 30.45 kB
oss-stable/react-reconciler/cjs/react-reconciler.profiling.min.js +0.28% 99.30 kB 99.58 kB +0.15% 30.40 kB 30.45 kB
oss-experimental/react-reconciler/cjs/react-reconciler.profiling.min.js +0.28% 101.77 kB 102.05 kB +0.20% 31.15 kB 31.22 kB
facebook-www/ReactDOM-profiling.modern.js +0.21% 425.98 kB 426.88 kB +0.17% 78.92 kB 79.05 kB
facebook-www/ReactDOMForked-profiling.modern.js +0.21% 425.98 kB 426.88 kB +0.17% 78.92 kB 79.05 kB
facebook-www/ReactDOM-profiling.classic.js +0.21% 437.47 kB 438.37 kB +0.19% 80.61 kB 80.76 kB
facebook-www/ReactDOMForked-profiling.classic.js +0.21% 437.47 kB 438.37 kB +0.18% 80.61 kB 80.76 kB
oss-stable-semver/react-dom/cjs/react-dom.profiling.min.js +0.20% 137.21 kB 137.49 kB +0.29% 43.65 kB 43.78 kB
oss-stable/react-dom/cjs/react-dom.profiling.min.js +0.20% 137.21 kB 137.49 kB +0.29% 43.65 kB 43.78 kB

Generated by 🚫 dangerJS against f4ab4e0

@bvaughn bvaughn force-pushed the scheduling-profiler-error-tracking branch from bf6a968 to f4ab4e0 Compare September 24, 2021 14:46
@bvaughn bvaughn merged commit af87f5a into facebook:main Sep 24, 2021
@bvaughn bvaughn deleted the scheduling-profiler-error-tracking branch September 24, 2021 16:32
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Oct 12, 2021
Summary:
This sync includes the following changes:
- **[579c008a7](facebook/react@579c008a7 )**: [Fizz/Flight] pipeToNodeWritable(..., writable).startWriting() -> renderToPipeableStream(...).pipe(writable) ([#22450](facebook/react#22450)) //<Sebastian Markbåge>//
- **[f2c381131](facebook/react@f2c381131 )**: fix: useSyncExternalStoreExtra ([#22500](facebook/react#22500)) //<Daishi Kato>//
- **[0ecbbe142](facebook/react@0ecbbe142 )**: Sync hydrate discrete events in capture phase and dont replay discrete events ([#22448](facebook/react#22448)) //<salazarm>//
- **[a724a3b57](facebook/react@a724a3b57 )**: [RFC] Codemod invariant -> throw new Error ([#22435](facebook/react#22435)) //<Andrew Clark>//
- **[201af81b0](facebook/react@201af81b0 )**: Release pooled cache reference in complete/unwind ([#22464](facebook/react#22464)) //<Joseph Savona>//
- **[033efe731](facebook/react@033efe731 )**: Call get snapshot in useSyncExternalStore server shim ([#22453](facebook/react#22453)) //<salazarm>//
- **[7843b142a](facebook/react@7843b142a )**: [Fizz/Flight] Pass in Destination lazily to startFlowing instead of in createRequest ([#22449](facebook/react#22449)) //<Sebastian Markbåge>//
- **[d9fb383d6](facebook/react@d9fb383d6 )**: Extract queueing logic into shared functions ([#22452](facebook/react#22452)) //<Andrew Clark>//
- **[9175f4d15](facebook/react@9175f4d15 )**: Scheduling Profiler: Show Suspense resource .displayName ([#22451](facebook/react#22451)) //<Brian Vaughn>//
- **[eba248c39](facebook/react@eba248c39 )**: [Fizz/Flight] Remove reentrancy hack ([#22446](facebook/react#22446)) //<Sebastian Markbåge>//
- **[66388150e](facebook/react@66388150e )**: Remove usereducer eager bailout ([#22445](facebook/react#22445)) //<Joseph Savona>//
- **[d3e086932](facebook/react@d3e086932 )**: Make root.unmount() synchronous  ([#22444](facebook/react#22444)) //<Andrew Clark>//
- **[2cc6d79c9](facebook/react@2cc6d79c9 )**: Rename onReadyToStream to onCompleteShell ([#22443](facebook/react#22443)) //<Sebastian Markbåge>//
- **[c88fb49d3](facebook/react@c88fb49d3 )**: Improve DEV errors if string coercion throws (Temporal.*, Symbol, etc.) ([#22064](facebook/react#22064)) //<Justin Grant>//
- **[05726d72c](facebook/react@05726d72c )**: [Fix] Errors should not "unsuspend" a transition ([#22423](facebook/react#22423)) //<Andrew Clark>//
- **[3746eaf98](facebook/react@3746eaf98 )**: Packages/React/src/ReactLazy ---> changing -1 to unintialized ([#22421](facebook/react#22421)) //<BIKI DAS>//
- **[04ccc01d9](facebook/react@04ccc01d9 )**: Hydration errors should force a client render ([#22416](facebook/react#22416)) //<Andrew Clark>//
- **[029fdcebb](facebook/react@029fdcebb )**: root.hydrate -> root.isDehydrated ([#22420](facebook/react#22420)) //<Andrew Clark>//
- **[af87f5a83](facebook/react@af87f5a83 )**: Scheduling Profiler marks should include thrown Errors ([#22417](facebook/react#22417)) //<Brian Vaughn>//
- **[d47339ea3](facebook/react@d47339ea3 )**: [Fizz] Remove assignID mechanism ([#22410](facebook/react#22410)) //<Sebastian Markbåge>//
- **[3a50d9557](facebook/react@3a50d9557 )**: Never attach ping listeners in legacy Suspense ([#22407](facebook/react#22407)) //<Andrew Clark>//
- **[82c8fa90b](facebook/react@82c8fa90b )**: Add back useMutableSource temporarily ([#22396](facebook/react#22396)) //<Andrew Clark>//
- **[5b57bc6e3](facebook/react@5b57bc6e3 )**: [Draft] don't patch console during first render ([#22308](facebook/react#22308)) //<Luna Ruan>//
- **[cf07c3df1](facebook/react@cf07c3df1 )**: Delete all but one `build2` reference ([#22391](facebook/react#22391)) //<Andrew Clark>//
- **[bb0d06935](facebook/react@bb0d06935 )**: [build2 -> build] Local scripts //<Andrew Clark>//
- **[0c81d347b](facebook/react@0c81d347b )**: Write artifacts to `build` instead of `build2` //<Andrew Clark>//
- **[4da03c9fb](facebook/react@4da03c9fb )**: useSyncExternalStore React Native version ([#22367](facebook/react#22367)) //<salazarm>//
- **[48d475c9e](facebook/react@48d475c9e )**: correct typos ([#22294](facebook/react#22294)) //<Bowen>//
- **[cb6c619c0](facebook/react@cb6c619c0 )**: Remove Fiber fields that were used for hydrating useMutableSource ([#22368](facebook/react#22368)) //<Sebastian Silbermann>//
- **[64e70f82e](facebook/react@64e70f82e )**: [Fizz] add avoidThisFallback support ([#22318](facebook/react#22318)) //<salazarm>//
- **[3ee7a004e](facebook/react@3ee7a004e )**: devtools: Display actual ReactDOM API name in root type ([#22363](facebook/react#22363)) //<Sebastian Silbermann>//
- **[79b8fc667](facebook/react@79b8fc667 )**: Implement getServerSnapshot in userspace shim ([#22359](facebook/react#22359)) //<Andrew Clark>//
- **[86b3e2461](facebook/react@86b3e2461 )**: Implement useSyncExternalStore on server ([#22347](facebook/react#22347)) //<Andrew Clark>//
- **[8209de269](facebook/react@8209de269 )**: Delete useMutableSource implementation ([#22292](facebook/react#22292)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions e8feb11...afcb9cd

jest_e2e[run_all_tests]

Reviewed By: yungsters

Differential Revision: D31541359

fbshipit-source-id: c35941bc303fdf55cb061e9996200dc868a6f2af
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
The scheduling profiler markComponentRenderStopped method is supposed to be called when rendering finishes or when a value is thrown (Suspense or Error). Previously we were calling this in a Suspense-only path of `throwException`.

This PR updates the code to handle errors (or non-Thenables) thrown as well.

It also moves the mark logic the work loop `handleError` method, with Suspense/Error agnostic cleanup.
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