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

[Transition Tracing] Refactor Transition Tracing Root Code #24766

Merged
merged 6 commits into from
Jun 30, 2022

Conversation

lunaruan
Copy link
Contributor

@lunaruan lunaruan commented Jun 21, 2022

This PR refactors the transition tracing root code by reusing the tracing marker code. Namely it:

  • Refactors the tracing marker code so that it takes a tracing marker instance instead of a tracing marker fiber and rename the stack to markerInstance instead of tracingMarker
  • Pushes the root code onto the stack
  • Moves the instantiation of root.incompleteTransitions to the begin phase when we are pushing the root to the stack rather than in the commit phase

Depends on #24686 only the refactor root commit should be looked at here

@lunaruan lunaruan requested a review from acdlite June 21, 2022 00:48
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jun 21, 2022
@sizebot
Copy link

sizebot commented Jun 21, 2022

Comparing: 88574c1...527baa8

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.03% 131.57 kB 131.61 kB +0.04% 42.28 kB 42.29 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.03% 136.84 kB 136.88 kB +0.03% 43.87 kB 43.89 kB
facebook-www/ReactDOM-prod.classic.js +0.13% 455.65 kB 456.26 kB +0.10% 82.90 kB 82.99 kB
facebook-www/ReactDOM-prod.modern.js +0.14% 440.88 kB 441.49 kB +0.10% 80.65 kB 80.73 kB
facebook-www/ReactDOMForked-prod.classic.js +0.13% 456.43 kB 457.04 kB +0.10% 83.13 kB 83.21 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactART-prod.modern.js +0.21% 283.33 kB 283.93 kB +0.15% 50.14 kB 50.21 kB
facebook-www/ReactART-prod.classic.js +0.20% 294.17 kB 294.77 kB +0.16% 51.93 kB 52.02 kB

Generated by 🚫 dangerJS against 527baa8

@lunaruan lunaruan changed the title [DevTools] Refactor Transition Tracing Root Code [Transition Tracing] Refactor Transition Tracing Root Code Jun 21, 2022
}

transitions.forEach(transition => {
incompleteTransitions.set(transition, new Map());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set the Map to null

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move some of this code to prepareFreshStack

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of pushing a Map() push a marker instance

pendingSuspenseBoundaries: PendingSuspenseBoundaries | null,
): void {
if (enableTransitionTracing) {
const markerInstance = {transitions, pendingSuspenseBoundaries};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't create just pass the actual instance

@@ -909,7 +910,10 @@ function updateTracingMarkerComponent(
}
}

pushTracingMarker(workInProgress);
const instance = workInProgress.stateNode;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to always cast to a specific type when reading from .stateNode

transitions.forEach(transition => {
// We need to create a new map here because we only have access to the
// object instance in the commit phase
root.incompleteTransitions.set(transition, {
Copy link
Collaborator

@acdlite acdlite Jun 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if this transition already exists in the map? Wouldn't this override it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah it shouldn't because we only push the root once and we only push the transition when it has just been added but I'll add a check here just to make sure!

@lunaruan lunaruan force-pushed the refactor_root branch 2 times, most recently from 2e6563f to ff6fa5e Compare June 28, 2022 20:50
packages/react-reconciler/src/ReactFiberBeginWork.new.js Outdated Show resolved Hide resolved
@@ -3702,7 +3714,10 @@ function attemptEarlyBailoutIfNoScheduledUpdate(
}
case TracingMarkerComponent: {
if (enableTransitionTracing) {
pushTracingMarker(workInProgress);
const instance: TracingMarkerInstance = workInProgress.stateNode;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const instance: TracingMarkerInstance = workInProgress.stateNode;
const instance: TracingMarkerInstance | null = workInProgress.stateNode;

packages/react-reconciler/src/ReactFiberWorkLoop.old.js Outdated Show resolved Hide resolved
@lunaruan lunaruan force-pushed the refactor_root branch 2 times, most recently from 6975b0b to a320d29 Compare June 29, 2022 17:09
@lunaruan lunaruan force-pushed the refactor_root branch 2 times, most recently from 28b39c5 to 2c6641b Compare June 29, 2022 20:07
@lunaruan lunaruan merged commit 4012963 into facebook:main Jun 30, 2022
acdlite added a commit to acdlite/react that referenced this pull request Jun 30, 2022
…acebook#24766)"

This reverts commit 4012963.

This PR refactors the transition tracing root code by reusing the tracing marker code. Namely it:
* Refactors the tracing marker code so that it takes a tracing marker instance instead of a tracing marker fiber and rename the stack to `markerInstance` instead of `tracingMarker`
* Pushes the root code onto the stack
* Moves the instantiation of `root.incompleteTransitions` to the begin phase when we are pushing the root to the stack rather than in the commit phase
acdlite added a commit to acdlite/react that referenced this pull request Jun 30, 2022
…acebook#24766)"

This reverts commit 4012963 because it's
failing on main, likely due to conflict with something that landed before the
PR was merged. Need to rebase and fix.
acdlite added a commit that referenced this pull request Jun 30, 2022
…24766)" (#24829)

This reverts commit 4012963 because it's
failing on main, likely due to conflict with something that landed before the
PR was merged. Need to rebase and fix.
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Sep 6, 2022
Summary:
This sync includes the following changes:
- **[c1f5884ff](facebook/react@c1f5884ff )**: Add missing null checks to OffscreenInstance code ([#24846](facebook/react#24846)) //<Andrew Clark>//
- **[4cd788aef](facebook/react@4cd788aef )**: Revert "Revert [Transition Tracing] Refactor Transition Tracing Root Code" ([#24830](facebook/react#24830)) //<Luna Ruan>//
- **[e61fd91f5](facebook/react@e61fd91f5 )**: Revert "[Transition Tracing] Refactor Transition Tracing Root Code ([#24766](facebook/react#24766))" ([#24829](facebook/react#24829)) //<Andrew Clark>//
- **[401296310](facebook/react@401296310 )**: [Transition Tracing] Refactor Transition Tracing Root Code ([#24766](facebook/react#24766)) //<Luna Ruan>//
- **[185932902](facebook/react@185932902 )**: Track nearest Suspense handler on stack ([#24585](facebook/react#24585)) //<Andrew Clark>//
- **[a7b192e0f](facebook/react@a7b192e0f )**: Add test gate alias for Offscreen ([#24749](facebook/react#24749)) //<Andrew Clark>//
- **[6b6cf8311](facebook/react@6b6cf8311 )**: Land forked reconciler changes ([#24817](facebook/react#24817)) //<Andrew Clark>//
- **[d1432ba93](facebook/react@d1432ba93 )**: [Transition Tracing] Fix excess calls to the transition start callback ([#24806](facebook/react#24806)) //<Luna Ruan>//
- **[88574c1b8](facebook/react@88574c1b8 )**: Fix enableTransitionTracing flag ([#24801](facebook/react#24801)) //<Luna Ruan>//
- **[a4bed4696](facebook/react@a4bed4696 )**: [Transition Tracing] Add Tracing Markers ([#24686](facebook/react#24686)) //<Luna Ruan>//
- **[167853026](facebook/react@167853026 )**: fix hydration warning suppression in text comparisons ([#24784](facebook/react#24784)) //<Josh Story>//
- **[9abe745aa](facebook/react@9abe745aa )**: [DevTools][Timeline Profiler] Component Stacks Backend ([#24776](facebook/react#24776)) //<Luna Ruan>//
- **[cf665c4b7](facebook/react@cf665c4b7 )**: [DevTools] Refactor incompleteTransitions field from Root Fiber memoized state to FiberRoot ([#24765](facebook/react#24765)) //<Luna Ruan>//
- **[56389e81f](facebook/react@56389e81f )**: Abort Flight ([#24754](facebook/react#24754)) //<Sebastian Markbåge>//
- **[0f216ae31](facebook/react@0f216ae31 )**: Add entry points for "static" server rendering passes ([#24752](facebook/react#24752)) //<Sebastian Markbåge>//
- **[f796fa13a](facebook/react@f796fa13a )**: Rename Segment to Task in Flight ([#24753](facebook/react#24753)) //<Sebastian Markbåge>//
- **[0f0aca3ab](facebook/react@0f0aca3ab )**: Aborting early should not infinitely suspend ([#24751](facebook/react#24751)) //<Sebastian Markbåge>//
- **[12a738f1a](facebook/react@12a738f1a )**: [Transition Tracing] Add Support for Multiple Transitions on Root ([#24732](facebook/react#24732)) //<Luna Ruan>//
- **[72ebc703a](facebook/react@72ebc703a )**: [DevTools] fix useDeferredValue to match reconciler change ([#24742](facebook/react#24742)) //<Mengdi Chen>//
- **[7cf9f5e03](facebook/react@7cf9f5e03 )**: Extra space ([#24612](facebook/react#24612)) //<Kerim Büyükakyüz>//

Changelog:
[General][Changed] - React Native sync for revisions 229c86a...c1f5884

Reviewed By: mdvacca, GijsWeterings

Differential Revision: D38904311

fbshipit-source-id: 1e30bc420c30ec7a0c0073fc92a706afef4b3340
cipolleschi pushed a commit to facebook/react-native that referenced this pull request Sep 7, 2022
Summary:
This sync includes the following changes:
- **[c1f5884ff](facebook/react@c1f5884ff )**: Add missing null checks to OffscreenInstance code ([#24846](facebook/react#24846)) //<Andrew Clark>//
- **[4cd788aef](facebook/react@4cd788aef )**: Revert "Revert [Transition Tracing] Refactor Transition Tracing Root Code" ([#24830](facebook/react#24830)) //<Luna Ruan>//
- **[e61fd91f5](facebook/react@e61fd91f5 )**: Revert "[Transition Tracing] Refactor Transition Tracing Root Code ([#24766](facebook/react#24766))" ([#24829](facebook/react#24829)) //<Andrew Clark>//
- **[401296310](facebook/react@401296310 )**: [Transition Tracing] Refactor Transition Tracing Root Code ([#24766](facebook/react#24766)) //<Luna Ruan>//
- **[185932902](facebook/react@185932902 )**: Track nearest Suspense handler on stack ([#24585](facebook/react#24585)) //<Andrew Clark>//
- **[a7b192e0f](facebook/react@a7b192e0f )**: Add test gate alias for Offscreen ([#24749](facebook/react#24749)) //<Andrew Clark>//
- **[6b6cf8311](facebook/react@6b6cf8311 )**: Land forked reconciler changes ([#24817](facebook/react#24817)) //<Andrew Clark>//
- **[d1432ba93](facebook/react@d1432ba93 )**: [Transition Tracing] Fix excess calls to the transition start callback ([#24806](facebook/react#24806)) //<Luna Ruan>//
- **[88574c1b8](facebook/react@88574c1b8 )**: Fix enableTransitionTracing flag ([#24801](facebook/react#24801)) //<Luna Ruan>//
- **[a4bed4696](facebook/react@a4bed4696 )**: [Transition Tracing] Add Tracing Markers ([#24686](facebook/react#24686)) //<Luna Ruan>//
- **[167853026](facebook/react@167853026 )**: fix hydration warning suppression in text comparisons ([#24784](facebook/react#24784)) //<Josh Story>//
- **[9abe745aa](facebook/react@9abe745aa )**: [DevTools][Timeline Profiler] Component Stacks Backend ([#24776](facebook/react#24776)) //<Luna Ruan>//
- **[cf665c4b7](facebook/react@cf665c4b7 )**: [DevTools] Refactor incompleteTransitions field from Root Fiber memoized state to FiberRoot ([#24765](facebook/react#24765)) //<Luna Ruan>//
- **[56389e81f](facebook/react@56389e81f )**: Abort Flight ([#24754](facebook/react#24754)) //<Sebastian Markbåge>//
- **[0f216ae31](facebook/react@0f216ae31 )**: Add entry points for "static" server rendering passes ([#24752](facebook/react#24752)) //<Sebastian Markbåge>//
- **[f796fa13a](facebook/react@f796fa13a )**: Rename Segment to Task in Flight ([#24753](facebook/react#24753)) //<Sebastian Markbåge>//
- **[0f0aca3ab](facebook/react@0f0aca3ab )**: Aborting early should not infinitely suspend ([#24751](facebook/react#24751)) //<Sebastian Markbåge>//
- **[12a738f1a](facebook/react@12a738f1a )**: [Transition Tracing] Add Support for Multiple Transitions on Root ([#24732](facebook/react#24732)) //<Luna Ruan>//
- **[72ebc703a](facebook/react@72ebc703a )**: [DevTools] fix useDeferredValue to match reconciler change ([#24742](facebook/react#24742)) //<Mengdi Chen>//
- **[7cf9f5e03](facebook/react@7cf9f5e03 )**: Extra space ([#24612](facebook/react#24612)) //<Kerim Büyükakyüz>//

Changelog:
[General][Changed] - React Native sync for revisions 229c86a...c1f5884

Reviewed By: mdvacca, GijsWeterings

Differential Revision: D38904311

fbshipit-source-id: 1e30bc420c30ec7a0c0073fc92a706afef4b3340
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This sync includes the following changes:
- **[c1f5884ff](facebook/react@c1f5884ff )**: Add missing null checks to OffscreenInstance code ([facebook#24846](facebook/react#24846)) //<Andrew Clark>//
- **[4cd788aef](facebook/react@4cd788aef )**: Revert "Revert [Transition Tracing] Refactor Transition Tracing Root Code" ([facebook#24830](facebook/react#24830)) //<Luna Ruan>//
- **[e61fd91f5](facebook/react@e61fd91f5 )**: Revert "[Transition Tracing] Refactor Transition Tracing Root Code ([facebook#24766](facebook/react#24766))" ([facebook#24829](facebook/react#24829)) //<Andrew Clark>//
- **[401296310](facebook/react@401296310 )**: [Transition Tracing] Refactor Transition Tracing Root Code ([facebook#24766](facebook/react#24766)) //<Luna Ruan>//
- **[185932902](facebook/react@185932902 )**: Track nearest Suspense handler on stack ([facebook#24585](facebook/react#24585)) //<Andrew Clark>//
- **[a7b192e0f](facebook/react@a7b192e0f )**: Add test gate alias for Offscreen ([facebook#24749](facebook/react#24749)) //<Andrew Clark>//
- **[6b6cf8311](facebook/react@6b6cf8311 )**: Land forked reconciler changes ([facebook#24817](facebook/react#24817)) //<Andrew Clark>//
- **[d1432ba93](facebook/react@d1432ba93 )**: [Transition Tracing] Fix excess calls to the transition start callback ([facebook#24806](facebook/react#24806)) //<Luna Ruan>//
- **[88574c1b8](facebook/react@88574c1b8 )**: Fix enableTransitionTracing flag ([facebook#24801](facebook/react#24801)) //<Luna Ruan>//
- **[a4bed4696](facebook/react@a4bed4696 )**: [Transition Tracing] Add Tracing Markers ([facebook#24686](facebook/react#24686)) //<Luna Ruan>//
- **[167853026](facebook/react@167853026 )**: fix hydration warning suppression in text comparisons ([facebook#24784](facebook/react#24784)) //<Josh Story>//
- **[9abe745aa](facebook/react@9abe745aa )**: [DevTools][Timeline Profiler] Component Stacks Backend ([facebook#24776](facebook/react#24776)) //<Luna Ruan>//
- **[cf665c4b7](facebook/react@cf665c4b7 )**: [DevTools] Refactor incompleteTransitions field from Root Fiber memoized state to FiberRoot ([facebook#24765](facebook/react#24765)) //<Luna Ruan>//
- **[56389e81f](facebook/react@56389e81f )**: Abort Flight ([facebook#24754](facebook/react#24754)) //<Sebastian Markbåge>//
- **[0f216ae31](facebook/react@0f216ae31 )**: Add entry points for "static" server rendering passes ([facebook#24752](facebook/react#24752)) //<Sebastian Markbåge>//
- **[f796fa13a](facebook/react@f796fa13a )**: Rename Segment to Task in Flight ([facebook#24753](facebook/react#24753)) //<Sebastian Markbåge>//
- **[0f0aca3ab](facebook/react@0f0aca3ab )**: Aborting early should not infinitely suspend ([facebook#24751](facebook/react#24751)) //<Sebastian Markbåge>//
- **[12a738f1a](facebook/react@12a738f1a )**: [Transition Tracing] Add Support for Multiple Transitions on Root ([facebook#24732](facebook/react#24732)) //<Luna Ruan>//
- **[72ebc703a](facebook/react@72ebc703a )**: [DevTools] fix useDeferredValue to match reconciler change ([facebook#24742](facebook/react#24742)) //<Mengdi Chen>//
- **[7cf9f5e03](facebook/react@7cf9f5e03 )**: Extra space ([facebook#24612](facebook/react#24612)) //<Kerim Büyükakyüz>//

Changelog:
[General][Changed] - React Native sync for revisions 229c86a...c1f5884

Reviewed By: mdvacca, GijsWeterings

Differential Revision: D38904311

fbshipit-source-id: 1e30bc420c30ec7a0c0073fc92a706afef4b3340
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.

4 participants