@samitouri / QOS-React-2 / commits / c3b18571db

[DevTools][Bugfix] Fix DevTools Perf Issue When Unmounting Large React Subtrees (#24863)

We've recently had multiple reports where, if React DevTools was installed, unmounting large React subtrees would take a huge performance hit (ex. from 50ms to 7 seconds). Digging in more, we realized for every fiber that unmounts, we called `untrackFibers`, which calls `clearTimeout` (and does some work manipulating a set, but this wasn't the bulk of the time). We ten call `recordUnmount`, which adds the timer back. Adding and removing the timer so many times was taking upwards of 50ms per timer add/remove call, which was resulting in exorbitant amounts of time spent in DevTools deleting subtrees. It looks like we are calling `untrackFibers` so many times to avoid a race condition with Suspense children where we unmount them twice (first a "virtual" unmount when the suspense boundary is toggled from visible to invisible, and then an actual unmount when the new children are rendered) without modifying `fiberIDMap`. We can fix this race condition by using the `untrackFibersSet` as a lock and not calling `recordUnmount` if the fiber is in the set and hasn't been processed yet. This works because the only way fibers are added in the set is via `recordUnmount` anyway. This PR also adds a test to make sure this change doesn't regress the previous behavior. **Before** ![image](https://user-images.githubusercontent.com/2735514/177655428-774ee306-0568-49ce-987e-b5213b613265.png) **After** ![image](https://user-images.githubusercontent.com/2735514/177655604-a217583f-787e-438e-b6f9-18953fe32444.png)

Luna Ruan committed Jul 7, 2022 at 10:43 UTC c3b18571dbf27b47675619aefc9cc9a4a06dbe18

2 files changed +53 -8

packages/react-devtools-shared/src/__tests__/store-test.js

+44

@@ -13,6 +13,7 @@ describe('Store', () => {

13 let ReactDOMClient;

14 let agent;

15 let act;

16 + let actAsync;

17 let bridge;

18 let getRendererID;

19 let legacyRender;

@@ -30,6 +31,7 @@ describe('Store', () => {

32 const utils = require('./utils');

33 act = utils.act;

34 + actAsync = utils.actAsync;

35 getRendererID = utils.getRendererID;

36 legacyRender = utils.legacyRender;

37 withErrorsOrWarningsIgnored = utils.withErrorsOrWarningsIgnored;

@@ -2064,5 +2066,47 @@ describe('Store', () => {

2066 expect(store.errorCount).toBe(0);

2067 expect(store.warningCount).toBe(0);

2068 });

2069 +

2070 + // Regression test for https://github.com/facebook/react/issues/23202

2071 + // @reactVersion >= 18.0

2072 + it('suspense boundary children should not double unmount and error', async () => {

2073 + async function fakeImport(result) {

2074 + return {default: result};

2075 + }

2076 +

2077 + const ChildA = () => null;

2078 + const ChildB = () => null;

2079 +

2080 + const LazyChildA = React.lazy(() => fakeImport(ChildA));

2081 + const LazyChildB = React.lazy(() => fakeImport(ChildB));

2082 +

2083 + function App({renderA}) {

2084 + return (

2085 + <React.Suspense>

2086 + {renderA ? <LazyChildA /> : <LazyChildB />}

2087 + </React.Suspense>

2088 + );

2089 + }

2090 +

2091 + const container = document.createElement('div');

2092 + const root = ReactDOMClient.createRoot(container);

2093 + await actAsync(() => root.render(<App renderA={true} />));

2094 +

2095 + expect(store).toMatchInlineSnapshot(`

2096 + [root]

2097 + ▾ <App>

2098 + ▾ <Suspense>

2099 + <ChildA>

2100 + `);

2101 +

2102 + await actAsync(() => root.render(<App renderA={false} />));

2103 +

2104 + expect(store).toMatchInlineSnapshot(`

2105 + [root]

2106 + ▾ <App>

2107 + ▾ <Suspense>

2108 + <ChildB>

2109 + `);

2110 + });

2111 });

2112 });

packages/react-devtools-shared/src/backend/renderer.js

+9 -8

@@ -2632,14 +2632,15 @@ export function attach(

2632 }

2633

2634 function handleCommitFiberUnmount(fiber) {

2635 - // Flush any pending Fibers that we are untracking before processing the new commit.

2636 - // If we don't do this, we might end up double-deleting Fibers in some cases (like Legacy Suspense).

2637 - untrackFibers();

2638 -

2639 - // This is not recursive.

2640 - // We can't traverse fibers after unmounting so instead

2641 - // we rely on React telling us about each unmount.

2642 - recordUnmount(fiber, false);

2635 + // If the untrackFiberSet already has the unmounted Fiber, this means we've already

2636 + // recordedUnmount, so we don't need to do it again. If we don't do this, we might

2637 + // end up double-deleting Fibers in some cases (like Legacy Suspense).

2638 + if (!untrackFibersSet.has(fiber)) {

2639 + // This is not recursive.

2640 + // We can't traverse fibers after unmounting so instead

2641 + // we rely on React telling us about each unmount.

2642 + recordUnmount(fiber, false);

2643 + }

2644 }

2645

2646 function handlePostCommitFiberRoot(root) {