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

Scheduling Profiler marks should include thrown Errors #22417

Merged
merged 1 commit into from
Sep 24, 2021

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.

None yet

4 participants