[DevTools][Transition Tracing] onTransitionComplete and onTransitionStart implmentation #23313

lunaruan · 2022-02-16T19:31:31Z

No description provided.

sizebot · 2022-02-16T19:36:21Z

Comparing: 68cb55f...47800dc

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	+0.05%	131.26 kB	131.33 kB	+0.05%	41.99 kB	42.01 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	+0.03%	136.12 kB	136.16 kB	+0.03%	43.42 kB	43.43 kB
facebook-www/ReactDOM-prod.classic.js	+0.01%	434.31 kB	434.37 kB	+0.01%	79.42 kB	79.43 kB
facebook-www/ReactDOM-prod.modern.js	+0.01%	424.09 kB	424.16 kB	+0.02%	77.98 kB	77.99 kB
facebook-www/ReactDOMForked-prod.classic.js	+0.01%	434.31 kB	434.37 kB	+0.01%	79.42 kB	79.43 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 47800dc

acdlite · 2022-02-17T15:28:54Z

packages/react-noop-renderer/src/createReactNoop.js

+      callbacks: TransitionTracingCallbacks,
+    ): void {
+      const endTime = Scheduler.unstable_now();
+      Scheduler.unstable_scheduleCallback(Scheduler.unstable_IdlePriority, () =>


If the only thing this does is call Scheduler, we might as well call Scheduler directly from the reconciler instead of going through the host config. We can add a host config once it becomes necessary.

Same for getCurrentEventStartTime, I think. Just call Scheduler.unstable_now for now.

acdlite · 2022-02-17T15:35:30Z

packages/react/src/ReactCurrentBatchConfig.js


 type BatchConfig = {
  transition: number,
  _updatedFibers?: Set<Fiber>,
+  transitionInfo: Transition | null,


Let's combine this with the transition field. We don't use the number for anything currently, we just check if it's set with currentBatchConfig.transition === 0. We can update those to currentBatchConfig.transition === null instead.

Then in all those places where you temporarily unset both transition and transitionInfo, you only need to unset a single field.

acdlite · 2022-02-17T15:46:04Z

packages/react-reconciler/src/ReactFiberCommitWork.new.js

+  if (enableTransitionTracing) {
+    switch (finishedWork.tag) {
+      case HostRoot: {
+        const currentTransitions = getTransitionsForLanes(root, lanes);


Is this the same as current.state.transitions?

Do we wnat to log this in startTransition later?

acdlite · 2022-02-17T15:48:12Z

packages/react-reconciler/src/ReactFiberCommitWork.new.js

@@ -984,7 +995,10 @@ function commitLayoutEffectOnFiber(
      case ScopeComponent:
      case OffscreenComponent:
      case LegacyHiddenComponent:
+      case TracingMarkerComponent: {


There's still some TracingMarkerComponent stuff in here that we don't need yet

lunaruan · 2022-02-17T16:26:24Z

packages/react-reconciler/src/__tests__/ReactTransitionTracing-test.js

+  beforeEach(() => {
+    jest.resetModules();
+    const ReactFeatureFlags = require('shared/ReactFeatureFlags');
+    ReactFeatureFlags.enableTransitionTracing = true;


Write in a way where you can test using the actual compiled bundles

// @GATE enableTransitionTracing

ReactFeatureFlags.www-dynamic -> put flags there

acdlite · 2022-02-23T20:16:26Z

packages/react-reconciler/src/ReactFiberBeginWork.old.js

        const cache: Cache = current.memoizedState.cache;
        pushCacheProvider(workInProgress, cache);
        pushRootCachePool(root);
      }
+      if (enableTransitionTracing) {
+        workInProgress.memoizedState.transitions = getTransitionsForLanes(


I understand why this loopy lane thing is necessary but it's a bit unfortunate. I think the ideal is that we'd have only a single loopy lane thing at the very beginning of the render phase. So, the loop that does entanglement stuff that currently happens in getNextLanes should be the same loop that creates this transition set, too. This will require some refactoring that is outside the scope of this PR, but we should try to align with that overall structure.

So, maybe you could move this getTransitionsForLanes call to the place in renderRootConcurrent and renderRootSync that calls prepareFreshStack:

react/packages/react-reconciler/src/ReactFiberWorkLoop.new.js

Lines 1692 to 1710 in 4035157

if (workInProgressRoot !== root || workInProgressRootRenderLanes !== lanes) {

if (enableUpdaterTracking) {

if (isDevToolsPresent) {

const memoizedUpdaters = root.memoizedUpdaters;

if (memoizedUpdaters.size > 0) {

restorePendingUpdaters(root, workInProgressRootRenderLanes);

memoizedUpdaters.clear();

}

// At this point, move Fibers that scheduled the upcoming work from the Map to the Set.

// If we bailout on this work, we'll move them back (like above).

// It's important to move them now in case the work spawns more work at the same priority with different updaters.

// That way we can keep the current update and future updates separate.

movePendingFibersToMemoized(root, lanes);

}

}

resetRenderTimer();

prepareFreshStack(root, lanes);

That block is the entry point into every new render phase. You would store the transitions in a work loop variable, something like workInProgressTransitions.

Later we can refactor to combine all the loopy lane stuff into a single loopy lane loop.

Then, in this function here, you would call getWorkInProgressTransitions, which does nothing except read the work loop variable.

acdlite · 2022-02-23T20:17:03Z

packages/react-reconciler/src/ReactFiberCommitWork.new.js

+      case HostRoot: {
+        const state = finishedWork.memoizedState;
+        const transitions = state.transitions;
+        if (transitions != null) {


Nit: !== null

acdlite · 2022-02-23T20:22:03Z

packages/react-reconciler/src/ReactFiberRoot.new.js

+export type RootState = {
+  element: any,
+  cache?: Cache | null,
+  transitions?: Transitions | null,


Let's just make both of these fields required. No ?, just Cache | null and Transitions | null. That way the value is never undefined.

acdlite · 2022-02-23T20:22:31Z

packages/react-reconciler/src/ReactFiberRoot.new.js

@@ -85,6 +93,10 @@ function FiberRootNode(

  if (enableTransitionTracing) {
    this.transitionCallbacks = null;
+    const transitionLanesMap = (this.transitionLanes = []);
+    for (let i = 0; i < TotalLanes; i++) {
+      transitionLanesMap.push(new Set());


This should initialize to null, not an empty set

acdlite · 2022-02-23T20:24:01Z

packages/react-reconciler/src/ReactFiberTracingMarkerComponent.new.js

  _updatedFibers?: Set<Fiber>,
 };
+
+export type Transitions = Set<Transition> | null;


Maybe this should be an array instead of a Set? The total size is usually 1, maybe 2 or 3 sometimes, right?

acdlite · 2022-02-23T20:26:00Z

packages/react-reconciler/src/ReactFiberTracingMarkerComponent.old.js

+  type: TransitionCallback,
+  transitionName: string,
+  startTime: number,
+  markerName?: string,


Why are these optional fields? They should always exist, or be null.

acdlite · 2022-02-23T20:29:12Z

packages/react-reconciler/src/ReactFiberCommitWork.new.js

+          transitions.forEach(transition => {
+            // TODO(luna) Do we want to log TransitionStart in the startTransition callback instead?
+            addCallbackToPendingTransitionCallbacks({
+              type: TransitionStart,


Why is this type field necessary? Why do you know it's a start callback here but not later?

It's so we know when/what to call the startTransitionCallback with. Do you recommend we do this some other way?

Add a transitionName to start transition, store the transition start time and name in the batch config, and pass it to the root on render

The root operates as a tracing marker that has all transitions on it. This PR only tested the root with one transition so far - Store transitions in memoizedState. Do this in updateHostRoot AND attemptEarlyBailoutIfNoScheduledUpdate. We need to do this in the latter part because even if the root itself doesn't have an update, it could still have new transitions in its transitionLanes map that we need to process.

- adds a module scoped pending transition callbacks object that contains all transition callbacks that have not yet been processed. This contains all callbacks before the next paint occurs. - Add code in the mutation phase to: * For the root, if there are transitions that were initialized during this commit in the root transition lanes map, add a transition start call to the pending transition callbacks object. Then, remove the transitions from the root transition lanes map. * For roots, in the commit phase, add a transition complete call We add this code in the mutation phase because we can't add it to the passive phase because then the paint might have occurred before we even know which callbacks to call

At the end of the commit phase, call scheduleTransitionCallbacks to schedule all pending transition callbacks to be called after paint. Then clear the callbacks

Summary: This sync includes the following changes: - **[17806594c](facebook/react@17806594c )**: Move createRoot/hydrateRoot to react-dom/client ([#23385](facebook/react#23385)) //<Sebastian Markbåge>// - **[75662d6a7](facebook/react@75662d6a7 )**: Remove hacky stream.locked check, declare as byte stream instead ([#23387](facebook/react#23387)) //<Sebastian Markbåge>// - **[a82ef6d40](facebook/react@a82ef6d40 )**: Add back skipUnmountedBoundaries flag only for www ([#23383](facebook/react#23383)) //<Andrew Clark>// - **[f468816ef](facebook/react@f468816ef )**: Fix false positive hydration warnings ([#23364](facebook/react#23364)) //<Andrew Clark>// - **[5d08a24c2](facebook/react@5d08a24c2 )**: useId: Use 'H' to separate main id from hook index ([#23363](facebook/react#23363)) //<Andrew Clark>// - **[3a60844a0](facebook/react@3a60844a0 )**: Update error message for suspending at sync priority ([#23361](facebook/react#23361)) //<Andrew Clark>// - **[efe4121ee](facebook/react@efe4121ee )**: Add : to beginning and end of every useId ([#23360](facebook/react#23360)) //<Andrew Clark>// - **[42f15b324](facebook/react@42f15b324 )**: [DevTools][Transition Tracing] onTransitionComplete and onTransitionStart implmentation ([#23313](facebook/react#23313)) //<Luna Ruan>// - **[a5b22155c](facebook/react@a5b22155c )**: Warn if renderSubtreeIntoContainer is called ([#23355](facebook/react#23355)) //<Andrew Clark>// - **[52c393b5d](facebook/react@52c393b5d )**: Revert to client render on text mismatch ([#23354](facebook/react#23354)) //<Andrew Clark>// - **[1ad8d8129](facebook/react@1ad8d8129 )**: Remove object-assign polyfill ([#23351](facebook/react#23351)) //<Sebastian Markbåge>// - **[b3f3da205](facebook/react@b3f3da205 )**: Land warnOnSubscriptionInsideStartTransition flag ([#23353](facebook/react#23353)) //<Andrew Clark>// - **[990098f88](facebook/react@990098f88 )**: Re-arrange main ReactFeatureFlags module ([#23350](facebook/react#23350)) //<Andrew Clark>// - **[1f3f6db73](facebook/react@1f3f6db73 )**: Remove createMutableSource from stable exports ([#23352](facebook/react#23352)) //<Andrew Clark>// - **[587e75930](facebook/react@587e75930 )**: Remove Numeric Fallback of Symbols ([#23348](facebook/react#23348)) //<Sebastian Markbåge>// - **[40351575d](facebook/react@40351575d )**: Split writeChunk into void and return value ([#23343](facebook/react#23343)) //<Sebastian Markbåge>// - **[2c693b2de](facebook/react@2c693b2de )**: Re-add reentrancy avoidance ([#23342](facebook/react#23342)) //<Sebastian Markbåge>// - **[1760b27c0](facebook/react@1760b27c0 )**: Remove ./src/* export from public build ([#23262](facebook/react#23262)) //<Andrew Clark>// - **[552c067bb](facebook/react@552c067bb )**: Remove public export for unstable-shared-subset.js ([#23261](facebook/react#23261)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 4de99b3...1780659 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D34552175 fbshipit-source-id: f1c831a45f96d335a20c3b4113196e0a42cefc03

…tart implmentation (facebook#23313) * add transition name to startTransition Add a transitionName to start transition, store the transition start time and name in the batch config, and pass it to the root on render * Transition Tracing Types and Consts * Root begin work The root operates as a tracing marker that has all transitions on it. This PR only tested the root with one transition so far - Store transitions in memoizedState. Do this in updateHostRoot AND attemptEarlyBailoutIfNoScheduledUpdate. We need to do this in the latter part because even if the root itself doesn't have an update, it could still have new transitions in its transitionLanes map that we need to process. * Transition Tracing commit phase - adds a module scoped pending transition callbacks object that contains all transition callbacks that have not yet been processed. This contains all callbacks before the next paint occurs. - Add code in the mutation phase to: * For the root, if there are transitions that were initialized during this commit in the root transition lanes map, add a transition start call to the pending transition callbacks object. Then, remove the transitions from the root transition lanes map. * For roots, in the commit phase, add a transition complete call We add this code in the mutation phase because we can't add it to the passive phase because then the paint might have occurred before we even know which callbacks to call * Process Callbacks after paint At the end of the commit phase, call scheduleTransitionCallbacks to schedule all pending transition callbacks to be called after paint. Then clear the callbacks

Summary: This sync includes the following changes: - **[17806594c](facebook/react@17806594c )**: Move createRoot/hydrateRoot to react-dom/client ([facebook#23385](facebook/react#23385)) //<Sebastian Markbåge>// - **[75662d6a7](facebook/react@75662d6a7 )**: Remove hacky stream.locked check, declare as byte stream instead ([facebook#23387](facebook/react#23387)) //<Sebastian Markbåge>// - **[a82ef6d40](facebook/react@a82ef6d40 )**: Add back skipUnmountedBoundaries flag only for www ([facebook#23383](facebook/react#23383)) //<Andrew Clark>// - **[f468816ef](facebook/react@f468816ef )**: Fix false positive hydration warnings ([facebook#23364](facebook/react#23364)) //<Andrew Clark>// - **[5d08a24c2](facebook/react@5d08a24c2 )**: useId: Use 'H' to separate main id from hook index ([facebook#23363](facebook/react#23363)) //<Andrew Clark>// - **[3a60844a0](facebook/react@3a60844a0 )**: Update error message for suspending at sync priority ([facebook#23361](facebook/react#23361)) //<Andrew Clark>// - **[efe4121ee](facebook/react@efe4121ee )**: Add : to beginning and end of every useId ([facebook#23360](facebook/react#23360)) //<Andrew Clark>// - **[42f15b324](facebook/react@42f15b324 )**: [DevTools][Transition Tracing] onTransitionComplete and onTransitionStart implmentation ([facebook#23313](facebook/react#23313)) //<Luna Ruan>// - **[a5b22155c](facebook/react@a5b22155c )**: Warn if renderSubtreeIntoContainer is called ([facebook#23355](facebook/react#23355)) //<Andrew Clark>// - **[52c393b5d](facebook/react@52c393b5d )**: Revert to client render on text mismatch ([facebook#23354](facebook/react#23354)) //<Andrew Clark>// - **[1ad8d8129](facebook/react@1ad8d8129 )**: Remove object-assign polyfill ([facebook#23351](facebook/react#23351)) //<Sebastian Markbåge>// - **[b3f3da205](facebook/react@b3f3da205 )**: Land warnOnSubscriptionInsideStartTransition flag ([facebook#23353](facebook/react#23353)) //<Andrew Clark>// - **[990098f88](facebook/react@990098f88 )**: Re-arrange main ReactFeatureFlags module ([facebook#23350](facebook/react#23350)) //<Andrew Clark>// - **[1f3f6db73](facebook/react@1f3f6db73 )**: Remove createMutableSource from stable exports ([facebook#23352](facebook/react#23352)) //<Andrew Clark>// - **[587e75930](facebook/react@587e75930 )**: Remove Numeric Fallback of Symbols ([facebook#23348](facebook/react#23348)) //<Sebastian Markbåge>// - **[40351575d](facebook/react@40351575d )**: Split writeChunk into void and return value ([facebook#23343](facebook/react#23343)) //<Sebastian Markbåge>// - **[2c693b2de](facebook/react@2c693b2de )**: Re-add reentrancy avoidance ([facebook#23342](facebook/react#23342)) //<Sebastian Markbåge>// - **[1760b27c0](facebook/react@1760b27c0 )**: Remove ./src/* export from public build ([facebook#23262](facebook/react#23262)) //<Andrew Clark>// - **[552c067bb](facebook/react@552c067bb )**: Remove public export for unstable-shared-subset.js ([facebook#23261](facebook/react#23261)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 4de99b3...1780659 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D34552175 fbshipit-source-id: f1c831a45f96d335a20c3b4113196e0a42cefc03

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

lunaruan force-pushed the onTransitionComplete branch 4 times, most recently from 54457ae to 2d2b5de Compare February 16, 2022 22:18

acdlite reviewed Feb 17, 2022

View reviewed changes

lunaruan commented Feb 17, 2022

View reviewed changes

lunaruan force-pushed the onTransitionComplete branch 3 times, most recently from 448f013 to e72a310 Compare February 18, 2022 04:27

acdlite reviewed Feb 23, 2022

View reviewed changes

acdlite approved these changes Feb 24, 2022

View reviewed changes

lunaruan force-pushed the onTransitionComplete branch from 18877f5 to de2bddc Compare February 24, 2022 22:16

lunaruan added 7 commits February 24, 2022 17:18

add transition name to startTransition

8b2d306

Add a transitionName to start transition, store the transition start time and name in the batch config, and pass it to the root on render

Transition Tracing Types and Consts

7fb60be

Process Callbacks after paint

1694a48

At the end of the commit phase, call scheduleTransitionCallbacks to schedule all pending transition callbacks to be called after paint. Then clear the callbacks

test

bf13715

fix lint errors

3b123ad

lunaruan added 2 commits February 24, 2022 17:18

old fork

a550e7f

addressed comments

47800dc

lunaruan force-pushed the onTransitionComplete branch from de2bddc to 47800dc Compare February 24, 2022 22:18

lunaruan merged commit 42f15b3 into facebook:main Feb 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DevTools][Transition Tracing] onTransitionComplete and onTransitionStart implmentation #23313

[DevTools][Transition Tracing] onTransitionComplete and onTransitionStart implmentation #23313

lunaruan commented Feb 16, 2022

sizebot commented Feb 16, 2022 •

edited

Loading

acdlite Feb 17, 2022

acdlite Feb 17, 2022

acdlite Feb 17, 2022

lunaruan Feb 17, 2022

acdlite Feb 17, 2022

lunaruan Feb 17, 2022

lunaruan Feb 17, 2022

acdlite Feb 23, 2022 •

edited

Loading

acdlite Feb 23, 2022

acdlite Feb 23, 2022

acdlite Feb 23, 2022

acdlite Feb 23, 2022

acdlite Feb 23, 2022

acdlite Feb 23, 2022

lunaruan Feb 24, 2022

	if (workInProgressRoot !== root \|\| workInProgressRootRenderLanes !== lanes) {
	if (enableUpdaterTracking) {
	if (isDevToolsPresent) {
	const memoizedUpdaters = root.memoizedUpdaters;
	if (memoizedUpdaters.size > 0) {
	restorePendingUpdaters(root, workInProgressRootRenderLanes);
	memoizedUpdaters.clear();
	}

	// At this point, move Fibers that scheduled the upcoming work from the Map to the Set.
	// If we bailout on this work, we'll move them back (like above).
	// It's important to move them now in case the work spawns more work at the same priority with different updaters.
	// That way we can keep the current update and future updates separate.
	movePendingFibersToMemoized(root, lanes);
	}
	}

	resetRenderTimer();
	prepareFreshStack(root, lanes);

[DevTools][Transition Tracing] onTransitionComplete and onTransitionStart implmentation #23313

[DevTools][Transition Tracing] onTransitionComplete and onTransitionStart implmentation #23313

Conversation

lunaruan commented Feb 16, 2022

sizebot commented Feb 16, 2022 • edited Loading

Critical size changes

Significant size changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sizebot commented Feb 16, 2022 •

edited

Loading

acdlite Feb 23, 2022 •

edited

Loading