Move all markRef calls into begin phase #28375

acdlite · 2024-02-18T20:46:21Z

Certain fiber types may have a ref attached to them. The main ones are HostComponent and ClassComponent. During the render phase, we check if a ref was passed to it, and if so, we schedule a Ref effect: markRef.

Currently, we're not consistent about whether we call markRef in the begin phase or the complete phase. For some fiber types, I found that markRef was called in both phases, causing redundant work.

After some investigation, I don't believe it's necessary to call markRef in both the begin phase and the complete phase, as long as you don't bail out before calling markRef.

I though that maybe it had to do with the attemptEarlyBailoutIfNoScheduledUpdates branch, which is a fast path that skips the regular begin phase if no new props, state, or context were passed. But if the props haven't changed (referentially — the memo and shouldComponentUpdate checks happen later), then it follows that the ref couldn't have changed either. This is true even in the old createElement runtime where ref is stored on the element instead of as a prop, because there's no way to pass a new ref to an element without also passing new props. You might argue this is a leaky assumption, but since we're shifting ref to be just a regular prop anyway, I think it's the correct way to think about it going forward.

I think the pattern of calling markRef in the complete phase may have been left over from an earlier iteration of the implementation before the bailout logic was structured like it is today.

So, I removed all the markRef calls from the complete phase. In the case of ScopeComponent, which had no corresponding call in the begin phase, I added one.

We already had a test that asserted that a ref is reattached even if the component bails out, but I added some new ones to be extra safe.

The reason I'm changing this this is because I'm working on a different change to move the ref handling logic in coerceRef to happen in render phase of the component that accepts the ref, instead of during the parent's reconciliation.

react-sizebot · 2024-02-18T20:50:18Z

Comparing: 59831c9...0dae8b5

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	=	177.01 kB	176.85 kB	=	55.17 kB	55.14 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	179.00 kB	178.85 kB	=	55.74 kB	55.71 kB
facebook-www/ReactDOM-prod.classic.js	=	592.58 kB	591.91 kB	=	104.54 kB	104.47 kB
facebook-www/ReactDOM-prod.modern.js	=	575.87 kB	575.20 kB	=	101.55 kB	101.47 kB
test_utils/ReactAllWarnings.js	Deleted	66.79 kB	0.00 kB	Deleted	16.36 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	66.79 kB	0.00 kB	Deleted	16.36 kB	0.00 kB

Generated by 🚫 dangerJS against 0dae8b5

Certain fiber types may have a ref attached to them. The main ones are HostComponent and ClassComponent. During the render phase, we check if a ref was passed to it, and if so, we schedule a Ref effect: `markRef`. Currently, we're not consistent about whether we call `markRef` in the begin phase or the complete phase. For some fiber types, I found that `markRef` was called in both phases, causing redundant work. After some investigation, I don't believe it's necessary to call `markRef` in both the begin phase and the complete phase, as long as you don't bail out before calling `markRef`. I though that maybe it had to do with the `attemptEarlyBailoutIfNoScheduledUpdates` branch, which is a fast path that skips the regular begin phase if no new props, state, or context were passed. But if the props haven't changed (referentially — the `memo` and `shouldComponentUpdate` checks happen later), then it follows that the ref couldn't have changed either. This is true even in the old `createElement` runtime where `ref` is stored on the element instead of as a prop, because there's no way to pass a new ref to an element without also passing new props. You might argue this is a leaky assumption, but since we're shifting ref to be just a regular prop anyway, I think it's the correct way to think about it going forward. I think the pattern of calling `markRef` in the complete phase may have been left over from an earlier iteration of the implementation before the bailout logic was structured like it is today. So, I removed all the `markRef` calls from the complete phase. In the case of ScopeComponent, which had no corresponding call in the begin phase, I added one. We already had a test that asserted that a ref is reattached even if the component bails out, but I added some new ones to be extra safe. The reason I'm changing this this is because I'm working on a different change to move the ref handling logic in `coerceRef` to happen in render phase of the component that accepts the ref, instead of during the parent's reconciliation.

sebmarkbage

seemslegit

Certain fiber types may have a ref attached to them. The main ones are HostComponent and ClassComponent. During the render phase, we check if a ref was passed to it, and if so, we schedule a Ref effect: `markRef`. Currently, we're not consistent about whether we call `markRef` in the begin phase or the complete phase. For some fiber types, I found that `markRef` was called in both phases, causing redundant work. After some investigation, I don't believe it's necessary to call `markRef` in both the begin phase and the complete phase, as long as you don't bail out before calling `markRef`. I though that maybe it had to do with the `attemptEarlyBailoutIfNoScheduledUpdates` branch, which is a fast path that skips the regular begin phase if no new props, state, or context were passed. But if the props haven't changed (referentially — the `memo` and `shouldComponentUpdate` checks happen later), then it follows that the ref couldn't have changed either. This is true even in the old `createElement` runtime where `ref` is stored on the element instead of as a prop, because there's no way to pass a new ref to an element without also passing new props. You might argue this is a leaky assumption, but since we're shifting ref to be just a regular prop anyway, I think it's the correct way to think about it going forward. I think the pattern of calling `markRef` in the complete phase may have been left over from an earlier iteration of the implementation before the bailout logic was structured like it is today. So, I removed all the `markRef` calls from the complete phase. In the case of ScopeComponent, which had no corresponding call in the begin phase, I added one. We already had a test that asserted that a ref is reattached even if the component bails out, but I added some new ones to be extra safe. The reason I'm changing this this is because I'm working on a different change to move the ref handling logic in `coerceRef` to happen in render phase of the component that accepts the ref, instead of during the parent's reconciliation. DiffTrain build for [c820097](c820097)

@acdlite

Certain fiber types may have a ref attached to them. The main ones are HostComponent and ClassComponent. During the render phase, we check if a ref was passed to it, and if so, we schedule a Ref effect: `markRef`. Currently, we're not consistent about whether we call `markRef` in the begin phase or the complete phase. For some fiber types, I found that `markRef` was called in both phases, causing redundant work. After some investigation, I don't believe it's necessary to call `markRef` in both the begin phase and the complete phase, as long as you don't bail out before calling `markRef`. I though that maybe it had to do with the `attemptEarlyBailoutIfNoScheduledUpdates` branch, which is a fast path that skips the regular begin phase if no new props, state, or context were passed. But if the props haven't changed (referentially — the `memo` and `shouldComponentUpdate` checks happen later), then it follows that the ref couldn't have changed either. This is true even in the old `createElement` runtime where `ref` is stored on the element instead of as a prop, because there's no way to pass a new ref to an element without also passing new props. You might argue this is a leaky assumption, but since we're shifting ref to be just a regular prop anyway, I think it's the correct way to think about it going forward. I think the pattern of calling `markRef` in the complete phase may have been left over from an earlier iteration of the implementation before the bailout logic was structured like it is today. So, I removed all the `markRef` calls from the complete phase. In the case of ScopeComponent, which had no corresponding call in the begin phase, I added one. We already had a test that asserted that a ref is reattached even if the component bails out, but I added some new ones to be extra safe. The reason I'm changing this this is because I'm working on a different change to move the ref handling logic in `coerceRef` to happen in render phase of the component that accepts the ref, instead of during the parent's reconciliation.

Certain fiber types may have a ref attached to them. The main ones are HostComponent and ClassComponent. During the render phase, we check if a ref was passed to it, and if so, we schedule a Ref effect: `markRef`. Currently, we're not consistent about whether we call `markRef` in the begin phase or the complete phase. For some fiber types, I found that `markRef` was called in both phases, causing redundant work. After some investigation, I don't believe it's necessary to call `markRef` in both the begin phase and the complete phase, as long as you don't bail out before calling `markRef`. I though that maybe it had to do with the `attemptEarlyBailoutIfNoScheduledUpdates` branch, which is a fast path that skips the regular begin phase if no new props, state, or context were passed. But if the props haven't changed (referentially — the `memo` and `shouldComponentUpdate` checks happen later), then it follows that the ref couldn't have changed either. This is true even in the old `createElement` runtime where `ref` is stored on the element instead of as a prop, because there's no way to pass a new ref to an element without also passing new props. You might argue this is a leaky assumption, but since we're shifting ref to be just a regular prop anyway, I think it's the correct way to think about it going forward. I think the pattern of calling `markRef` in the complete phase may have been left over from an earlier iteration of the implementation before the bailout logic was structured like it is today. So, I removed all the `markRef` calls from the complete phase. In the case of ScopeComponent, which had no corresponding call in the begin phase, I added one. We already had a test that asserted that a ref is reattached even if the component bails out, but I added some new ones to be extra safe. The reason I'm changing this this is because I'm working on a different change to move the ref handling logic in `coerceRef` to happen in render phase of the component that accepts the ref, instead of during the parent's reconciliation. DiffTrain build for commit c820097.

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

acdlite force-pushed the move-markref-to-begin-phase branch from 661654e to 0dae8b5 Compare February 19, 2024 16:42

acdlite marked this pull request as ready for review February 19, 2024 16:45

acdlite requested a review from sebmarkbage February 19, 2024 17:20

sebmarkbage approved these changes Feb 19, 2024

View reviewed changes

acdlite merged commit c820097 into facebook:main Feb 20, 2024
36 checks passed

This was referenced Feb 20, 2024

Bump vendored react versions vercel/next.js#62302

Closed

Upgrade vendored react vercel/next.js#62326

Merged

ztanner mentioned this pull request Mar 19, 2024

(backport) Update React from 60a927d04 to 14898b6a9 vercel/next.js#63442

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move all markRef calls into begin phase #28375

Move all markRef calls into begin phase #28375

acdlite commented Feb 18, 2024 •

edited

Loading

react-sizebot commented Feb 18, 2024 •

edited

Loading

sebmarkbage left a comment

Move all markRef calls into begin phase #28375

Move all markRef calls into begin phase #28375

Conversation

acdlite commented Feb 18, 2024 • edited Loading

react-sizebot commented Feb 18, 2024 • edited Loading

Critical size changes

Significant size changes

sebmarkbage left a comment

Choose a reason for hiding this comment

acdlite commented Feb 18, 2024 •

edited

Loading

react-sizebot commented Feb 18, 2024 •

edited

Loading