DevTools: Fix "unknown" updater in profiler when a component unsuspends #28330

eps1lon · 2024-02-14T18:40:17Z

Completes #28005
Closes #22603

When we pinged a Wakeable we marked the root as updated without moving the pending updaters from the pinged lanes to the update lane. This resulted in the update be marked as "unknown" in devtools. This may have also caused leaking of pending updaters since we cleared the pending lanes while keeping the updaters around on these lanes. Now we transfer all updaters from the pendingLanes to the designated update lane.

Displaying the host root as the updater is still not as useful but that's another bug (see #29735).

Update: I think the actual bug is that we restore pending updaters on a ping instead of adding the source fiber to the pending updaters on the retry lane. Restoring pending updaters is essentially causing https://github.com/facebook/react/pull/29735/files#diff-4962849261d085a5fbf4389513ab1a3a9b442cc8e1c4629f130ba2e03ef2b6bfR382-R386. Not sure yet where to put that logic.

Test plan

related test uses new root now instead of legacy root
Build with this PR does not show "Unknown" updater when profiling:
Before:

After:

react-sizebot · 2024-02-14T18:44:11Z

Comparing: d77dd31...eb3f84b

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.js	=	6.66 kB	6.66 kB	=	1.82 kB	1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js	=	496.48 kB	496.45 kB	=	88.87 kB	88.86 kB
oss-experimental/react-dom/cjs/react-dom.production.js	=	6.67 kB	6.67 kB	+0.05%	1.83 kB	1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js	=	501.30 kB	501.27 kB	=	89.56 kB	89.55 kB
facebook-www/ReactDOM-prod.classic.js	=	593.97 kB	593.94 kB	=	104.48 kB	104.48 kB
facebook-www/ReactDOM-prod.modern.js	=	570.35 kB	570.33 kB	=	100.89 kB	100.88 kB
test_utils/ReactAllWarnings.js	Deleted	63.65 kB	0.00 kB	Deleted	15.90 kB	0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
test_utils/ReactAllWarnings.js	Deleted	63.65 kB	0.00 kB	Deleted	15.90 kB	0.00 kB

Generated by 🚫 dangerJS against eb3f84b

gnoff · 2024-02-27T22:56:52Z

@acdlite @kassens I was going to approve this test but noticed the assertions had to change with the modern root. Not really sure what this test is for but it looks like @kassens you omitted it from the original PR that converts to createRoot. Do either of you know if this needs to be maintained as a legacy mode test?

eps1lon · 2024-02-27T23:46:33Z

We get the old expect(allSchedulerTypes).toEqual([[null], [Suspender], [Suspender]]); match once we start looking into pendingUpdatersLaneMap with

diff --git a/packages/react-reconciler/src/__tests__/ReactUpdaters-test.internal.js b/packages/react-reconciler/src/__tests__/ReactUpdaters-test.internal.js
index d518159ce8..b45f7e6259 100644
--- a/packages/react-reconciler/src/__tests__/ReactUpdaters-test.internal.js
+++ b/packages/react-reconciler/src/__tests__/ReactUpdaters-test.internal.js
@@ -45,10 +45,19 @@ describe('updaters', () => {
         }
         const schedulerTags = [];
         const schedulerTypes = [];
-        fiberRoot.memoizedUpdaters.forEach(fiber => {
-          schedulerTags.push(fiber.tag);
-          schedulerTypes.push(fiber.elementType);
-        });
+        if (fiberRoot.memoizedUpdaters.size > 0) {
+          fiberRoot.memoizedUpdaters.forEach(fiber => {
+            schedulerTags.push(fiber.tag);
+            schedulerTypes.push(fiber.elementType);
+          });
+        } else {
+          fiberRoot.pendingUpdatersLaneMap.forEach(pendingUpdaters => {
+            pendingUpdaters.forEach(fiber => {
+              schedulerTags.push(fiber.tag);
+              schedulerTypes.push(fiber.elementType);
+            });
+          });
+        }
         allSchedulerTags.push(schedulerTags);
         allSchedulerTypes.push(schedulerTypes);
       }),

I'm just now piecing enableUpdaterTracking together. Maybe we're missing a movePendingFibersToMemoized call somewhere?

packages/react-reconciler/src/__tests__/ReactUpdaters-test.internal.js

vercel · 2024-06-02T20:13:18Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-compiler-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 2, 2024 10:38pm

eps1lon · 2024-06-02T20:16:37Z

packages/react-reconciler/src/ReactFiberLane.js

+  // TODO: When would we ever ping lanes that we aren't suspended on?
+  root.pingedLanes |= pingedLanes;


Do we need to investigate this further? Just transferring the updaters from root.pingedLanes to updateLane isn't enough. For some reason we're pinging when root.suspendedLanes & pingedLanes === NoLanes. Which seems odd intuitively but maybe the bug is root.suspendedLanes having no overlap with pingedLanes?

When would we ever ping lanes that we aren't suspended on?

Maybe if we suspend on two Wakeables A and B, A gets pinged earlier, we unsuspend completely i.e. don't suspend on B again (e.g. conditional use?) and then we wouldn't want to retry when B gets pinged?

Grasping for straws here since all tests still pass with this change.

eps1lon · 2024-06-02T20:24:31Z

packages/react-reconciler/src/__tests__/ReactUpdaters-test.internal.js

@@ -266,9 +274,6 @@ describe('updaters', () => {
    await waitForAll([]);
  });

-  // This test should be convertable to createRoot but the allScheduledTypes assertions are no longer the same
-  // So I'm leaving it in legacy mode for now and just disabling if legacy mode is turned off
-  // @gate !disableLegacyMode
  it('should cover suspense pings', async () => {


The original issue in the test was that the updater was on the Default lane, the updater got pinged, and then we said we should render on a Retry lane (markRootPinged). This left the updater dangling in pending updaters since we only move the pending updaters to memoized on the lane we actually render (Retry lane).

eps1lon · 2024-06-02T21:33:25Z

packages/react-reconciler/src/__tests__/ReactUpdaters-test.internal.js

+            // TODO: Is it ever ok to have dangling pending updaters or is this always a bug?
+            // const lane = 1 << index;
+            // throw new Error(
+            //   `Lane ${lane} has pending updaters. Either you didn't assert on all updates in your test or React is leaking updaters.`,
+            // );


Tests that currently would fail this: https://app.circleci.com/pipelines/github/facebook/react/54155/workflows/17c8ec84-c64b-4fc1-bde6-6fcb7532513e/jobs/892710

eps1lon · 2024-06-02T21:44:04Z

packages/react-reconciler/src/ReactFiberLane.js

@@ -625,6 +625,8 @@ export function markRootUpdated(root: FiberRoot, updateLane: Lane) {
  // idle updates until after all the regular updates have finished; there's no
  // way it could unblock a transition.
  if (updateLane !== IdleLane) {
+    movePendingUpdatersToLane(root, root.pingedLanes, updateLane);


I don't think we need to transfer updaters from root.suspendedLanes but I haven't really thought about this for more than a sec.

Fixes ```` SyntaxError: ~/react/packages/react-reconciler/src/ReactFiberLane.js: Compiling let/const in this block would add a closure (throwIfClosureRequired). 614 | if (updateLane !== IdleLane) { 615 | if (enableUpdaterTracking) { > 616 | if (isDevToolsPresent) { | ^ 617 | // transfer pending updaters from pingedLanes to updateLane 618 | const pendingUpdatersLaneMap = root.pendingUpdatersLaneMap; 619 | const updaters = pendingUpdatersLaneMap[laneToIndex(updateLane)]; ```

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 14, 2024

eps1lon marked this pull request as ready for review February 14, 2024 18:50

eps1lon mentioned this pull request Mar 1, 2024

[DOM] disable legacy mode behind flag #28468

Merged

eps1lon force-pushed the test/create-root/ReactUpdaters branch from ae87aca to 16ee439 Compare March 14, 2024 14:19

eps1lon commented Mar 14, 2024

View reviewed changes

packages/react-reconciler/src/__tests__/ReactUpdaters-test.internal.js Outdated Show resolved Hide resolved

Sebastian Silbermann and others added 2 commits June 2, 2024 20:39

Convert ReactUpdaters to createRoot

a9394fc

DevTools: Fix "unknown" updater in profiler when a component unsuspends

dcc5ca6

eps1lon force-pushed the test/create-root/ReactUpdaters branch from 16ee439 to dcc5ca6 Compare June 2, 2024 20:13

eps1lon changed the title ~~Convert ReactUpdaters to createRoot~~ DevTools: Fix "unknown" updater in profiler when a component unsuspends Jun 2, 2024

eps1lon commented Jun 2, 2024

View reviewed changes

vercel bot deployed to Preview June 2, 2024 20:18 View deployment

eps1lon commented Jun 2, 2024

View reviewed changes

Ignore dangling pending updaters for now

2abf6dc

eps1lon commented Jun 2, 2024

View reviewed changes

vercel bot deployed to Preview June 2, 2024 21:35 View deployment

eps1lon commented Jun 2, 2024

View reviewed changes

vercel bot deployed to Preview June 2, 2024 21:45 View deployment

eps1lon force-pushed the test/create-root/ReactUpdaters branch from 6a2add4 to ffd8102 Compare June 2, 2024 21:58

vercel bot deployed to Preview June 2, 2024 22:00 View deployment

eps1lon force-pushed the test/create-root/ReactUpdaters branch from ffd8102 to eb3f84b Compare June 2, 2024 22:35

vercel bot deployed to Preview June 2, 2024 22:38 View deployment

eps1lon mentioned this pull request Jun 3, 2024

Current behavior of "What caused this update?" to point to host root after a ping #29735

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DevTools: Fix "unknown" updater in profiler when a component unsuspends #28330

DevTools: Fix "unknown" updater in profiler when a component unsuspends #28330

eps1lon commented Feb 14, 2024 •

edited

Loading

react-sizebot commented Feb 14, 2024 •

edited

Loading

gnoff commented Feb 27, 2024

eps1lon commented Feb 27, 2024

vercel bot commented Jun 2, 2024 •

edited

Loading

eps1lon Jun 2, 2024 •

edited

Loading

eps1lon Jun 2, 2024 •

edited

Loading

eps1lon Jun 2, 2024

eps1lon Jun 2, 2024

eps1lon Jun 2, 2024

		// TODO: When would we ever ping lanes that we aren't suspended on?
		root.pingedLanes \|= pingedLanes;

DevTools: Fix "unknown" updater in profiler when a component unsuspends #28330

Are you sure you want to change the base?

DevTools: Fix "unknown" updater in profiler when a component unsuspends #28330

Conversation

eps1lon commented Feb 14, 2024 • edited Loading

Test plan

react-sizebot commented Feb 14, 2024 • edited Loading

Critical size changes

Significant size changes

gnoff commented Feb 27, 2024

eps1lon commented Feb 27, 2024

vercel bot commented Jun 2, 2024 • edited Loading

eps1lon Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

eps1lon Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

eps1lon Jun 2, 2024

Choose a reason for hiding this comment

eps1lon Jun 2, 2024

Choose a reason for hiding this comment

eps1lon Jun 2, 2024

Choose a reason for hiding this comment

eps1lon commented Feb 14, 2024 •

edited

Loading

react-sizebot commented Feb 14, 2024 •

edited

Loading

vercel bot commented Jun 2, 2024 •

edited

Loading

eps1lon Jun 2, 2024 •

edited

Loading

eps1lon Jun 2, 2024 •

edited

Loading