Remove firstEffect from Persistent Mode Optimization

[sebmarkbage]

Builds on top of #19322 with one commit e829f13

Persistent mode needs to clone a parent and add its children if a child has changed.

We have an optimization in persistent mode where we don't do that if no child could've changed. If there are no effects scheduled for any child then there couldn't have been changes.

Instead of checking for this on firstEffect, we now check this on the children's effectTag and subtreeTags.

This is quite unfortunate because if we could just do this check a little bit later we would've already gotten it transferred to the completed work's subtreeTag. Now we have to loop over all the children and if any of them changed, we have to loop over them again. Doing at least two loops per parent.

One possible fix to this would be to move the resetChildLanes to be an explicit call within each completeWork branch before they return. That way we could call it before returning in this case.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit dec061d:

Sandbox	Source
React	Configuration

[sizebot]

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against dec061d

[sizebot]

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against dec061d

[acdlite]

I'm trying to remember the reason why resetChildLanes exists. An alternative would be to reset the childLanes (and subtreeTag, and similar fields) to 0 in the begin phase, and accumulate the child values in each child fiber's complete phase. Instead of doing another traversal.

We already do something similar for the Incomplete effect tag, which essentially has the same properties as a subtreeTag since it is set on all parents of an incomplete fiber (something that threw):

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

Line 1746 in 392277f

returnFiber.effectTag |= Incomplete;

I'm thinking maybe the original reason has something to do with resuming? Maybe it's not relevant anymore?

If so, maybe we could get rid of resetChildLanes completely and move everything to completeWork.

[acdlite]

I was hoping to do something like this anyway to get rid of this Offscreen check that's currently sitting smack dab in the middle of the hot path:

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

Lines 1769 to 1777 in fed4ae0

	if (
	// TODO: Move this check out of the hot path by moving `resetChildLanes`
	// to switch statement in `completeWork`.
	(completedWork.tag === LegacyHiddenComponent ||
	completedWork.tag === OffscreenComponent) &&
	completedWork.memoizedState !== null &&
	!includesSomeLane(subtreeRenderLanes, (OffscreenLane: Lane)) &&
	(completedWork.mode & ConcurrentMode) !== NoLanes
	) {

[acdlite]

Lol next time I'll read the description before I comment:

One possible fix to this would be to move the resetChildLanes to be an explicit call within each completeWork branch before they return. That way we could call it before returning in this case.

Yeah this makes sense to me. Let's do it.

[bvaughn]

One possible fix to this would be to move the resetChildLanes to be an explicit call within each completeWork branch before they return. That way we could call it before returning in this case.

If so, maybe we could get rid of resetChildLanes completely and move everything to completeWork.

👍 This sounds like a good change. Who owns it? Want me to pick it up?

[bvaughn]

Makes sense. Will be nicer once we can just check the subtreeTag directly.

[sebmarkbage]

Accumulating the effectTag as we go is effectively what the effect list did. However that has some downsides too. It makes rendering something only to throw it out difficult. It also makes things order dependent.

Interestingly I think the new refactor will “fix” the suspense list issue by making effects fire in first-to-last child order instead of the reveal order as was before.

appendAllChildren is another one of those that would be nice to move to happen inside the children and append to the parent but it suffers from the same problem.

Moving it to the complete phase as a second pass should work though. It also allows passes to ignore it if it knows it doesn’t have to like if something is Offscreen. However it also requires us to remember to call it.

[acdlite]

Interestingly I think the new refactor will “fix” the suspense list issue by making effects fire in first-to-last child order instead of the reveal order as was before.

Yeah the key difference between the effect list and childLanes, subtreeTag, et al is that they aren't order dependent. And now that effect list is gone and we operate on the tree, effects aren't order dependent either.

[lunaruan]

Interestingly I think the new refactor will “fix” the suspense list issue by making effects fire in first-to-last child order instead of the reveal order as was before.

What is the suspense list issue you all are referring to?

[sebmarkbage]

What is the suspense list issue you all are referring to?

@lunaruan SuspenseList with revealOrder="backwards" caused the effects of siblings to fire backwards. I.e. mount effects in a list of A, B, C would fire mount for C first, then B, then A. That's fine because we don't really guarantee it but that was the only case that happened.