Pass lanes as argument to performSyncWorkOnRoot (#26763)

`performSyncWorkOnRoot` has only a single caller, and the caller already
computes the next lanes (`getNextLanes`) before deciding to call the
function. So we can pass them as an argument instead of computing the
lanes again.

There was already a TODO comment about this, but it was mostly perf
related. However, @rickhanlonii noticed a discrepancy where the inner
`getNextLanes` call was not being passed the current work-in- progress
lanes. Usually this shouldn't matter because there should never be
work-in-progress sync work; it should finish immediately. There is one
case I'm aware of where we exit the work loop without finishing a sync
render, which is selective hydration, but even then it should switch to
the sync hydration lane, not the normal sync lane. So something else is
probably going on. I suspect it might be related to the
`enableUnifiedSyncLane` experiment.

This is likely related to a regression found internally at Meta. We're
still working on getting a proper regression test; I can come up with a
contrived one but I'm not confident it'll be the same as the actual
regression until we get a better repro.
This commit is contained in:
Andrew Clark
2023-09-28 16:29:32 -04:00
committed by GitHub
parent e2589d8488
commit bc2a62d690
2 changed files with 17 additions and 17 deletions
+4 -6
View File
@@ -164,9 +164,6 @@ function flushSyncWorkAcrossRoots_impl(onlyLegacy: boolean) {
return;
}
const workInProgressRoot = getWorkInProgressRoot();
const workInProgressRootRenderLanes = getWorkInProgressRootRenderLanes();
// There may or may not be synchronous work scheduled. Let's check.
let didPerformSomeWork;
let errors: Array<mixed> | null = null;
@@ -178,6 +175,9 @@ function flushSyncWorkAcrossRoots_impl(onlyLegacy: boolean) {
if (onlyLegacy && root.tag !== LegacyRoot) {
// Skip non-legacy roots.
} else {
const workInProgressRoot = getWorkInProgressRoot();
const workInProgressRootRenderLanes =
getWorkInProgressRootRenderLanes();
const nextLanes = getNextLanes(
root,
root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes,
@@ -185,10 +185,8 @@ function flushSyncWorkAcrossRoots_impl(onlyLegacy: boolean) {
if (includesSyncLane(nextLanes)) {
// This root has pending sync work. Flush it now.
try {
// TODO: Pass nextLanes as an argument instead of computing it again
// inside performSyncWorkOnRoot.
didPerformSomeWork = true;
performSyncWorkOnRoot(root);
performSyncWorkOnRoot(root, nextLanes);
} catch (error) {
// Collect errors so we can rethrow them at the end
if (errors === null) {
+13 -11
View File
@@ -1254,25 +1254,27 @@ function markRootSuspended(root: FiberRoot, suspendedLanes: Lanes) {
// This is the entry point for synchronous tasks that don't go
// through Scheduler
export function performSyncWorkOnRoot(root: FiberRoot): null {
if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
syncNestedUpdateFlag();
}
export function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes): null {
if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
throw new Error('Should not already be working.');
}
flushPassiveEffects();
// TODO: This was already computed in the caller. Pass it as an argument.
let lanes = getNextLanes(root, NoLanes);
if (!includesSyncLane(lanes)) {
// There's no remaining sync work left.
const didFlushPassiveEffects = flushPassiveEffects();
if (didFlushPassiveEffects) {
// If passive effects were flushed, exit to the outer work loop in the root
// scheduler, so we can recompute the priority.
// TODO: We don't actually need this `ensureRootIsScheduled` call because
// this path is only reachable if the root is already part of the schedule.
// I'm including it only for consistency with the other exit points from
// this function. Can address in a subsequent refactor.
ensureRootIsScheduled(root);
return null;
}
if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
syncNestedUpdateFlag();
}
let exitStatus = renderRootSync(root, lanes);
if (root.tag !== LegacyRoot && exitStatus === RootErrored) {
// If something threw an error, try rendering one more time. We'll render