@samitouri / QOS-React-1 / commits / 13d0225c7d

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.

Andrew Clark committed Sep 28, 2023 at 16:29 UTC 13d0225c7d4715d98772a85d8deb26d244921287
2 files changed +17 -17
packages/react-reconciler/src/ReactFiberRootScheduler.js
+4 -6
@@ -164,9 +164,6 @@ function flushSyncWorkAcrossRoots_impl(onlyLegacy: boolean) {
164 return;
165 }
166
167 - const workInProgressRoot = getWorkInProgressRoot();
168 - const workInProgressRootRenderLanes = getWorkInProgressRootRenderLanes();
169 -
167 // There may or may not be synchronous work scheduled. Let's check.
168 let didPerformSomeWork;
169 let errors: Array<mixed> | null = null;
@@ -178,6 +175,9 @@ function flushSyncWorkAcrossRoots_impl(onlyLegacy: boolean) {
175 if (onlyLegacy && root.tag !== LegacyRoot) {
176 // Skip non-legacy roots.
177 } else {
178 + const workInProgressRoot = getWorkInProgressRoot();
179 + const workInProgressRootRenderLanes =
180 + getWorkInProgressRootRenderLanes();
181 const nextLanes = getNextLanes(
182 root,
183 root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes,
@@ -185,10 +185,8 @@ function flushSyncWorkAcrossRoots_impl(onlyLegacy: boolean) {
185 if (includesSyncLane(nextLanes)) {
186 // This root has pending sync work. Flush it now.
187 try {
188 - // TODO: Pass nextLanes as an argument instead of computing it again
189 - // inside performSyncWorkOnRoot.
188 didPerformSomeWork = true;
191 - performSyncWorkOnRoot(root);
189 + performSyncWorkOnRoot(root, nextLanes);
190 } catch (error) {
191 // Collect errors so we can rethrow them at the end
192 if (errors === null) {
packages/react-reconciler/src/ReactFiberWorkLoop.js
+13 -11
@@ -1254,25 +1254,27 @@ function markRootSuspended(root: FiberRoot, suspendedLanes: Lanes) {
1254
1255 // This is the entry point for synchronous tasks that don't go
1256 // through Scheduler
1257 -export function performSyncWorkOnRoot(root: FiberRoot): null {
1258 - if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
1259 - syncNestedUpdateFlag();
1260 - }
1261 -
1257 +export function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes): null {
1258 if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
1259 throw new Error('Should not already be working.');
1260 }
1261
1266 - flushPassiveEffects();
1267 -
1268 - // TODO: This was already computed in the caller. Pass it as an argument.
1269 - let lanes = getNextLanes(root, NoLanes);
1270 - if (!includesSyncLane(lanes)) {
1271 - // There's no remaining sync work left.
1262 + const didFlushPassiveEffects = flushPassiveEffects();
1263 + if (didFlushPassiveEffects) {
1264 + // If passive effects were flushed, exit to the outer work loop in the root
1265 + // scheduler, so we can recompute the priority.
1266 + // TODO: We don't actually need this `ensureRootIsScheduled` call because
1267 + // this path is only reachable if the root is already part of the schedule.
1268 + // I'm including it only for consistency with the other exit points from
1269 + // this function. Can address in a subsequent refactor.
1270 ensureRootIsScheduled(root);
1271 return null;
1272 }
1273
1274 + if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
1275 + syncNestedUpdateFlag();
1276 + }
1277 +
1278 let exitStatus = renderRootSync(root, lanes);
1279 if (root.tag !== LegacyRoot && exitStatus === RootErrored) {
1280 // If something threw an error, try rendering one more time. We'll render