@samitouri / QOS-React / commits / d83601080a

Wrap retrySuspendedRoot using SchedulerTracing (#13776)

Previously, we were emptying root.pendingInteractionMap and permanently losing those interactions when applying an unrelated update to a tree that has no scheduled work that is waiting on promise resolution. (That is, one that is showing a fallback and waiting for the suspended content to resolve.) The logic I'm leaving untouched with `nextRenderIncludesTimedOutPlaceholder` is *not* correct -- what we want is instead to know if *any* placeholder anywhere in the tree is showing its fallback -- but we don't currently have a better replacement, and this should unblock tracing with suspense again.

Sophie Alpert committed Oct 4, 2018 at 15:11 UTC d83601080a7c913cedcfbad86044702d008039c7

4 files changed +122 -129

packages/react-reconciler/src/ReactFiberBeginWork.js

-9

@@ -43,14 +43,12 @@ import {

43 Update,

44 Ref,

45 } from 'shared/ReactSideEffectTags';

46 -import {captureWillSyncRenderPlaceholder} from './ReactFiberScheduler';

46 import ReactSharedInternals from 'shared/ReactSharedInternals';

47 import {

48 enableSuspense,

49 debugRenderPhaseSideEffects,

50 debugRenderPhaseSideEffectsForStrictMode,

51 enableProfilerTimer,

53 - enableSchedulerTracing,

52 } from 'shared/ReactFeatureFlags';

53 import invariant from 'shared/invariant';

54 import shallowEqual from 'shared/shallowEqual';

@@ -952,13 +950,6 @@ function updatePlaceholderComponent(

950

951 let nextDidTimeout;

952 if (current !== null && workInProgress.updateQueue !== null) {

955 - if (enableSchedulerTracing) {

956 - // Handle special case of rendering a Placeholder for a sync, suspended tree.

957 - // We flag this to properly trace and count interactions.

958 - // Otherwise interaction pending count will be decremented too many times.

959 - captureWillSyncRenderPlaceholder();

960 - }

961 -

953 // We're outside strict mode. Something inside this Placeholder boundary

954 // suspended during the last commit. Switch to the placholder.

955 workInProgress.updateQueue = null;

packages/react-reconciler/src/ReactFiberScheduler.js

+81 -96

@@ -257,11 +257,6 @@ let legacyErrorBoundariesThatAlreadyFailed: Set<mixed> | null = null;

257 // Used for performance tracking.

258 let interruptedBy: Fiber | null = null;

259

260 -// Do not decrement interaction counts in the event of suspense timeouts.

261 -// This would lead to prematurely calling the interaction-complete hook.

262 -// Instead we wait until the suspended promise has resolved.

263 -let nextRenderIncludesTimedOutPlaceholder: boolean = false;

264 -

260 let stashedWorkInProgressProperties;

261 let replayUnitOfWork;

262 let isReplayingFailedUnitOfWork;

@@ -766,42 +761,40 @@ function commitRoot(root: FiberRoot, finishedWork: Fiber): void {

761 unhandledError = error;

762 }

763 } finally {

769 - if (!nextRenderIncludesTimedOutPlaceholder) {

770 - // Clear completed interactions from the pending Map.

771 - // Unless the render was suspended or cascading work was scheduled,

772 - // In which case– leave pending interactions until the subsequent render.

773 - const pendingInteractionMap = root.pendingInteractionMap;

774 - pendingInteractionMap.forEach(

775 - (scheduledInteractions, scheduledExpirationTime) => {

776 - // Only decrement the pending interaction count if we're done.

777 - // If there's still work at the current priority,

778 - // That indicates that we are waiting for suspense data.

779 - if (

780 - earliestRemainingTimeAfterCommit === NoWork ||

781 - scheduledExpirationTime < earliestRemainingTimeAfterCommit

782 - ) {

783 - pendingInteractionMap.delete(scheduledExpirationTime);

784 -

785 - scheduledInteractions.forEach(interaction => {

786 - interaction.__count--;

787 -

788 - if (subscriber !== null && interaction.__count === 0) {

789 - try {

790 - subscriber.onInteractionScheduledWorkCompleted(interaction);

791 - } catch (error) {

792 - // It's not safe for commitRoot() to throw.

793 - // Store the error for now and we'll re-throw in finishRendering().

794 - if (!hasUnhandledError) {

795 - hasUnhandledError = true;

796 - unhandledError = error;

797 - }

764 + // Clear completed interactions from the pending Map.

765 + // Unless the render was suspended or cascading work was scheduled,

766 + // In which case– leave pending interactions until the subsequent render.

767 + const pendingInteractionMap = root.pendingInteractionMap;

768 + pendingInteractionMap.forEach(

769 + (scheduledInteractions, scheduledExpirationTime) => {

770 + // Only decrement the pending interaction count if we're done.

771 + // If there's still work at the current priority,

772 + // That indicates that we are waiting for suspense data.

773 + if (

774 + earliestRemainingTimeAfterCommit === NoWork ||

775 + scheduledExpirationTime < earliestRemainingTimeAfterCommit

776 + ) {

777 + pendingInteractionMap.delete(scheduledExpirationTime);

778 +

779 + scheduledInteractions.forEach(interaction => {

780 + interaction.__count--;

781 +

782 + if (subscriber !== null && interaction.__count === 0) {

783 + try {

784 + subscriber.onInteractionScheduledWorkCompleted(interaction);

785 + } catch (error) {

786 + // It's not safe for commitRoot() to throw.

787 + // Store the error for now and we'll re-throw in finishRendering().

788 + if (!hasUnhandledError) {

789 + hasUnhandledError = true;

790 + unhandledError = error;

791 }

792 }

800 - });

801 - }

802 - },

803 - );

804 - }

793 + }

794 + });

795 + }

796 + },

797 + );

798 }

799 }

800 }

@@ -1173,11 +1166,6 @@ function renderRoot(

1166 root.pendingCommitExpirationTime = NoWork;

1167

1168 if (enableSchedulerTracing) {

1176 - // Reset this flag once we start rendering a new root or at a new priority.

1177 - // This might indicate that suspended work has completed.

1178 - // If not, the flag will be reset.

1179 - nextRenderIncludesTimedOutPlaceholder = false;

1180 -

1169 // Determine which interactions this batch of work currently includes,

1170 // So that we can accurately attribute time spent working on it,

1171 // And so that cascading work triggered during the render phase will be associated with it.

@@ -1388,9 +1376,6 @@ function renderRoot(

1376

1377 if (enableSuspense && !isExpired && nextLatestAbsoluteTimeoutMs !== -1) {

1378 // The tree was suspended.

1391 - if (enableSchedulerTracing) {

1392 - nextRenderIncludesTimedOutPlaceholder = true;

1393 - }

1379 const suspendedExpirationTime = expirationTime;

1380 markSuspendedPriorityLevel(root, suspendedExpirationTime);

1381

@@ -1597,6 +1582,13 @@ function retrySuspendedRoot(

1582 markPendingPriorityLevel(root, retryTime);

1583 }

1584

1585 + // TODO: If the placeholder fiber has already rendered the primary children

1586 + // without suspending (that is, all of the promises have already resolved),

1587 + // we should not trigger another update here. One case this happens is when

1588 + // we are in sync mode and a single promise is thrown both on initial render

1589 + // and on update; we attach two .then(retrySuspendedRoot) callbacks and each

1590 + // one performs Sync work, rerendering the Placeholder.

1591 +

1592 if ((fiber.mode & ConcurrentMode) !== NoContext) {

1593 if (root === nextRoot && nextRenderExpirationTime === suspendedTime) {

1594 // Received a ping at the same priority level at which we're currently

@@ -1614,6 +1606,15 @@ function retrySuspendedRoot(

1606 }

1607

1608 function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null {

1609 + recordScheduleUpdate();

1610 +

1611 + if (__DEV__) {

1612 + if (fiber.tag === ClassComponent || fiber.tag === ClassComponentLazy) {

1613 + const instance = fiber.stateNode;

1614 + warnAboutInvalidUpdates(instance);

1615 + }

1616 + }

1617 +

1618 // Update the source fiber's expiration time

1619 if (

1620 fiber.expirationTime === NoWork ||

@@ -1631,49 +1632,39 @@ function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null {

1632 }

1633 // Walk the parent path to the root and update the child expiration time.

1634 let node = fiber.return;

1635 + let root = null;

1636 if (node === null && fiber.tag === HostRoot) {

1635 - return fiber.stateNode;

1636 - }

1637 - while (node !== null) {

1638 - alternate = node.alternate;

1639 - if (

1640 - node.childExpirationTime === NoWork ||

1641 - node.childExpirationTime > expirationTime

1642 - ) {

1643 - node.childExpirationTime = expirationTime;

1637 + root = fiber.stateNode;

1638 + } else {

1639 + while (node !== null) {

1640 + alternate = node.alternate;

1641 if (

1642 + node.childExpirationTime === NoWork ||

1643 + node.childExpirationTime > expirationTime

1644 + ) {

1645 + node.childExpirationTime = expirationTime;

1646 + if (

1647 + alternate !== null &&

1648 + (alternate.childExpirationTime === NoWork ||

1649 + alternate.childExpirationTime > expirationTime)

1650 + ) {

1651 + alternate.childExpirationTime = expirationTime;

1652 + }

1653 + } else if (

1654 alternate !== null &&

1655 (alternate.childExpirationTime === NoWork ||

1656 alternate.childExpirationTime > expirationTime)

1657 ) {

1658 alternate.childExpirationTime = expirationTime;

1659 }

1651 - } else if (

1652 - alternate !== null &&

1653 - (alternate.childExpirationTime === NoWork ||

1654 - alternate.childExpirationTime > expirationTime)

1655 - ) {

1656 - alternate.childExpirationTime = expirationTime;

1657 - }

1658 - if (node.return === null && node.tag === HostRoot) {

1659 - return node.stateNode;

1660 - }

1661 - node = node.return;

1662 - }

1663 - return null;

1664 -}

1665 -

1666 -function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) {

1667 - recordScheduleUpdate();

1668 -

1669 - if (__DEV__) {

1670 - if (fiber.tag === ClassComponent || fiber.tag === ClassComponentLazy) {

1671 - const instance = fiber.stateNode;

1672 - warnAboutInvalidUpdates(instance);

1660 + if (node.return === null && node.tag === HostRoot) {

1661 + root = node.stateNode;

1662 + break;

1663 + }

1664 + node = node.return;

1665 }

1666 }

1667

1676 - const root = scheduleWorkToRoot(fiber, expirationTime);

1668 if (root === null) {

1669 if (

1670 __DEV__ &&

@@ -1681,7 +1672,7 @@ function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) {

1672 ) {

1673 warnAboutUpdateOnUnmounted(fiber);

1674 }

1684 - return;

1675 + return null;

1676 }

1677

1678 if (enableSchedulerTracing) {

@@ -1718,6 +1709,15 @@ function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) {

1709 }

1710 }

1711

1712 + return root;

1713 +}

1714 +

1715 +function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) {

1716 + const root = scheduleWorkToRoot(fiber, expirationTime);

1717 + if (root === null) {

1718 + return;

1719 + }

1720 +

1721 if (

1722 !isWorking &&

1723 nextRenderExpirationTime !== NoWork &&

@@ -1904,15 +1904,7 @@ function onTimeout(root, finishedWork, suspendedExpirationTime) {

1904 // because we're at the top of a timer event.

1905 recomputeCurrentRendererTime();

1906 currentSchedulerTime = currentRendererTime;

1907 -

1908 - if (enableSchedulerTracing) {

1909 - // Don't update pending interaction counts for suspense timeouts,

1910 - // Because we know we still need to do more work in this case.

1911 - nextRenderIncludesTimedOutPlaceholder = true;

1912 - flushRoot(root, suspendedExpirationTime);

1913 - } else {

1914 - flushRoot(root, suspendedExpirationTime);

1915 - }

1907 + flushRoot(root, suspendedExpirationTime);

1908 }

1909 }

1910

@@ -2480,12 +2472,6 @@ function flushControlled(fn: () => mixed): void {

2472 }

2473 }

2474

2483 -function captureWillSyncRenderPlaceholder() {

2484 - if (enableSchedulerTracing) {

2485 - nextRenderIncludesTimedOutPlaceholder = true;

2486 - }

2487 -}

2488 -

2475 export {

2476 requestCurrentTime,

2477 computeExpirationForFiber,

@@ -2508,5 +2494,4 @@ export {

2494 interactiveUpdates,

2495 flushInteractiveUpdates,

2496 computeUniqueAsyncExpiration,

2511 - captureWillSyncRenderPlaceholder,

2497 };

packages/react-reconciler/src/ReactFiberUnwindWork.js

+5 -9

@@ -14,6 +14,7 @@ import type {CapturedValue} from './ReactCapturedValue';

14 import type {Update} from './ReactUpdateQueue';

15 import type {Thenable} from './ReactFiberScheduler';

17 +import {unstable_wrap as Schedule_tracing_wrap} from 'scheduler/tracing';

18 import getComponentName from 'shared/getComponentName';

19 import warningWithoutStack from 'shared/warningWithoutStack';

20 import {

@@ -59,7 +60,6 @@ import {

60 markLegacyErrorBoundaryAsFailed,

61 isAlreadyFailedLegacyErrorBoundary,

62 retrySuspendedRoot,

62 - captureWillSyncRenderPlaceholder,

63 } from './ReactFiberScheduler';

64 import {Sync} from './ReactFiberExpirationTime';

@@ -224,12 +224,15 @@ function throwException(

224 : renderExpirationTime;

225

226 // Attach a listener to the promise to "ping" the root and retry.

227 - const onResolveOrReject = retrySuspendedRoot.bind(

227 + let onResolveOrReject = retrySuspendedRoot.bind(

228 null,

229 root,

230 workInProgress,

231 pingTime,

232 );

233 + if (enableSchedulerTracing) {

234 + onResolveOrReject = Schedule_tracing_wrap(onResolveOrReject);

235 + }

236 thenable.then(onResolveOrReject, onResolveOrReject);

237

238 // If the boundary is outside of strict mode, we should *not* suspend

@@ -243,13 +246,6 @@ function throwException(

246 if ((workInProgress.mode & StrictMode) === NoEffect) {

247 workInProgress.effectTag |= UpdateEffect;

248

246 - if (enableSchedulerTracing) {

247 - // Handles the special case of unwinding a suspended sync render.

248 - // We flag this to properly trace and count interactions.

249 - // Otherwise interaction pending count will be decremented too many times.

250 - captureWillSyncRenderPlaceholder();

251 - }

252 -

249 // Unmount the source fiber's children

250 const nextChildren = null;

251 reconcileChildren(

packages/react/src/__tests__/ReactProfiler-test.internal.js

+36 -15

@@ -2219,6 +2219,14 @@ describe('Profiler', () => {

2219 timestamp: mockNow(),

2220 };

2221

2222 + const monkey = React.createRef();

2223 + class Monkey extends React.Component {

2224 + render() {

2225 + ReactNoop.yield('Monkey');

2226 + return null;

2227 + }

2228 + }

2229 +

2230 const onRender = jest.fn();

2231 SchedulerTracing.unstable_trace(interaction.name, mockNow(), () => {

2232 ReactNoop.render(

@@ -2227,6 +2235,7 @@ describe('Profiler', () => {

2235 <AsyncText text="Async" ms={20000} />

2236 </React.Placeholder>

2237 <Text text="Sync" />

2238 + <Monkey ref={monkey} />

2239 </React.unstable_Profiler>,

2240 );

2241 });

@@ -2243,6 +2252,7 @@ describe('Profiler', () => {

2252 'Suspend [Async]',

2253 'Text [Loading...]',

2254 'Text [Sync]',

2255 + 'Monkey',

2256 ]);

2257 // The update hasn't expired yet, so we commit nothing.

2258 expect(ReactNoop.getChildren()).toEqual([]);

@@ -2271,6 +2281,11 @@ describe('Profiler', () => {

2281 expect(onInteractionTraced).toHaveBeenCalledTimes(1);

2282 expect(onInteractionScheduledWorkCompleted).not.toHaveBeenCalled();

2283

2284 + // An unrelated update in the middle shouldn't affect things...

2285 + monkey.current.forceUpdate();

2286 + expect(ReactNoop.flush()).toEqual(['Monkey']);

2287 + expect(onRender).toHaveBeenCalledTimes(2);

2288 +

2289 // Once the promise resolves, we render the suspended view

2290 await awaitableAdvanceTimers(10000);

2291 expect(ReactNoop.flush()).toEqual([

@@ -2281,9 +2296,9 @@ describe('Profiler', () => {

2296 {text: 'Async'},

2297 {text: 'Sync'},

2298 ]);

2284 - expect(onRender).toHaveBeenCalledTimes(2);

2299 + expect(onRender).toHaveBeenCalledTimes(3);

2300

2286 - call = onRender.mock.calls[1];

2301 + call = onRender.mock.calls[2];

2302 expect(call[0]).toEqual('test-profiler');

2303 expect(call[6]).toMatchInteractions(

2304 ReactFeatureFlags.enableSchedulerTracing ? [interaction] : [],

@@ -2540,7 +2555,9 @@ describe('Profiler', () => {

2555

2556 expect(onRender).toHaveBeenCalledTimes(2); // Sync null commit, placeholder commit

2557 expect(onRender.mock.calls[0][6]).toMatchInteractions([

2543 - initialRenderInteraction,

2558 + highPriUpdateInteraction,

2559 + ]);

2560 + expect(onRender.mock.calls[1][6]).toMatchInteractions([

2561 highPriUpdateInteraction,

2562 ]);

2563 onRender.mockClear();

@@ -2552,9 +2569,12 @@ describe('Profiler', () => {

2569 await originalPromise;

2570 expect(renderer.toJSON()).toEqual(['loaded', 'updated']);

2571

2572 + // TODO: Bug. This *should* just be one render tied to both interactions.

2573 expect(onRender).toHaveBeenCalledTimes(2);

2574 expect(onRender.mock.calls[0][6]).toMatchInteractions([

2575 initialRenderInteraction,

2576 + ]);

2577 + expect(onRender.mock.calls[1][6]).toMatchInteractions([

2578 highPriUpdateInteraction,

2579 ]);

2580

@@ -2631,6 +2651,11 @@ describe('Profiler', () => {

2651 },

2652 );

2653 });

2654 + expect(ReactTestRenderer).toHaveYielded([

2655 + 'Suspend [loaded]',

2656 + 'Text [loading]',

2657 + 'Text [updated]',

2658 + ]);

2659 expect(renderer.toJSON()).toEqual(['loading', 'updated']);

2660

2661 expect(onRender).toHaveBeenCalledTimes(1);

@@ -2639,32 +2664,28 @@ describe('Profiler', () => {

2664 ]);

2665 onRender.mockClear();

2666

2642 - expect(onInteractionScheduledWorkCompleted).toHaveBeenCalledTimes(1);

2643 - expect(

2644 - onInteractionScheduledWorkCompleted,

2645 - ).toHaveBeenLastNotifiedOfInteraction(highPriUpdateInteraction);

2667 + expect(onInteractionScheduledWorkCompleted).toHaveBeenCalledTimes(0);

2668

2669 advanceTimeBy(500);

2670 jest.advanceTimersByTime(500);

2671 await originalPromise;

2650 - expect(ReactTestRenderer).toHaveYielded([

2651 - 'Suspend [loaded]',

2652 - 'Text [loading]',

2653 - 'Text [updated]',

2654 - 'Promise resolved [loaded]',

2655 - ]);

2672 + expect(ReactTestRenderer).toHaveYielded(['Promise resolved [loaded]']);

2673 expect(renderer).toFlushAndYield(['AsyncText [loaded]']);

2674 expect(renderer.toJSON()).toEqual(['loaded', 'updated']);

2675

2676 expect(onRender).toHaveBeenCalledTimes(1);

2677 expect(onRender.mock.calls[0][6]).toMatchInteractions([

2678 initialRenderInteraction,

2679 + highPriUpdateInteraction,

2680 ]);

2681

2682 expect(onInteractionScheduledWorkCompleted).toHaveBeenCalledTimes(2);

2683 expect(

2666 - onInteractionScheduledWorkCompleted,

2667 - ).toHaveBeenLastNotifiedOfInteraction(initialRenderInteraction);

2684 + onInteractionScheduledWorkCompleted.mock.calls[0][0],

2685 + ).toMatchInteraction(initialRenderInteraction);

2686 + expect(

2687 + onInteractionScheduledWorkCompleted.mock.calls[1][0],

2688 + ).toMatchInteraction(highPriUpdateInteraction);

2689 });

2690 });

2691 });