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
Move guarded callback closer to usage #21658
Conversation
Comparing: c153679...626cd53 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
@@ -423,121 +450,106 @@ function commitBeforeMutationEffects_complete() { | |||
} | |||
|
|||
function commitBeforeMutationEffectsOnFiber(finishedWork: Fiber) { | |||
const current = finishedWork.alternate; | |||
const flags = finishedWork.flags; | |||
setCurrentDebugFiberInDEV(finishedWork); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I like the removal of the Snapshot
flag check around the switch statement. It seems a little weird to call a method that does not sound (but happens to be ) snapshot-specific (commitBeforeMutationEffectsOnFiber
) and then do work without checking in that method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add the outer check (so we don't do the guarded callback unnecessarily) and leave the inner check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking at commitPassiveMountOnFiber
which has the check outside. Overall I think there's some tension in this code between having generic naming and avoiding too many guards. I don't know what's a great way to balance but repeating the same check twice in a hot path also seems non-ideal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. My initial impression was that moving the check outside was not great (b'c less clear) but I see what you're saying too. I don't have strong feelings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most fibers do not have a Snapshot flag set. It's a rare case. So with the guard, the most common case is a single check, then you bail out. Without the guard, you have to do at least one check (the switch
case) then additional checks for some branches (like class components).
So I would add back the Snapshot check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh there's a check in the outer function. Nvm, didn't see that part.
Diff between two branches (easiest way to review IMO): |
commitMutationEffectsOnFiber(fiber, root); | ||
} catch (error) { | ||
captureCommitPhaseError(fiber, fiber.return, error); | ||
if (fiber.flags !== NoFlags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (fiber.flags !== NoFlags) { | |
if ((fiber.flags & MutationMask) !== NoFlags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the one nit, LGTM
Oh haha I didn't see #21666. Even better! |
Based on #21657.
Changes:
(flags & Snapshot) !== NoFlags
check out of the inner function. Leaves the inner function in guarded callback. ThebeforeActiveInstanceBlur
stuff might need guarding in theory (though I'm not sure we're consistent about it now), so I wrapped it separately just to be safe.flags
areNoFlags
. I readfiber.flags
but should be ok because it's read inside right after anyway. All code inside only runs on non-zero flags so this should be safe.Why: so that we spend less time in DEV in these guarded callbacks that don't do anything. In particular, if Fiber has no effects.