Skip to content

Commit ab2c406

Browse files
committed
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. DiffTrain build for [13d0225](13d0225)
1 parent 20fecfd commit ab2c406

23 files changed

+2314
-2254
lines changed

compiled/facebook-www/REVISION

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
1ebedbec2bec08e07c286ea6c3cff62737a0fd3a
1+
13d0225c7d4715d98772a85d8deb26d244921287

compiled/facebook-www/React-dev.classic.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ if (
2727
}
2828
"use strict";
2929

30-
var ReactVersion = "18.3.0-www-classic-443dd2b0";
30+
var ReactVersion = "18.3.0-www-classic-f8c6554b";
3131

3232
// ATTENTION
3333
// When adding new symbols to this file,

compiled/facebook-www/React-prod.classic.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -623,4 +623,4 @@ exports.useSyncExternalStore = function (
623623
exports.useTransition = function () {
624624
return ReactCurrentDispatcher.current.useTransition();
625625
};
626-
exports.version = "18.3.0-www-classic-928934f2";
626+
exports.version = "18.3.0-www-classic-c257e462";

compiled/facebook-www/React-prod.modern.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -615,4 +615,4 @@ exports.useSyncExternalStore = function (
615615
exports.useTransition = function () {
616616
return ReactCurrentDispatcher.current.useTransition();
617617
};
618-
exports.version = "18.3.0-www-modern-0f3cd1d0";
618+
exports.version = "18.3.0-www-modern-1f62d21d";

compiled/facebook-www/React-profiling.modern.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -626,7 +626,7 @@ exports.useSyncExternalStore = function (
626626
exports.useTransition = function () {
627627
return ReactCurrentDispatcher.current.useTransition();
628628
};
629-
exports.version = "18.3.0-www-modern-cc976fcc";
629+
exports.version = "18.3.0-www-modern-9df16c08";
630630

631631
/* global __REACT_DEVTOOLS_GLOBAL_HOOK__ */
632632
if (

compiled/facebook-www/ReactART-dev.classic.js

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ function _assertThisInitialized(self) {
6969
return self;
7070
}
7171

72-
var ReactVersion = "18.3.0-www-classic-e5fc32c4";
72+
var ReactVersion = "18.3.0-www-classic-bface95b";
7373

7474
var LegacyRoot = 0;
7575
var ConcurrentRoot = 1;
@@ -7398,10 +7398,7 @@ function flushSyncWorkAcrossRoots_impl(onlyLegacy) {
73987398
if (!mightHavePendingSyncWork) {
73997399
// Fast path. There's no sync work to do.
74007400
return;
7401-
}
7402-
7403-
var workInProgressRoot = getWorkInProgressRoot();
7404-
var workInProgressRootRenderLanes = getWorkInProgressRootRenderLanes(); // There may or may not be synchronous work scheduled. Let's check.
7401+
} // There may or may not be synchronous work scheduled. Let's check.
74057402

74067403
var didPerformSomeWork;
74077404
var errors = null;
@@ -7414,6 +7411,8 @@ function flushSyncWorkAcrossRoots_impl(onlyLegacy) {
74147411
while (root !== null) {
74157412
if (onlyLegacy && root.tag !== LegacyRoot);
74167413
else {
7414+
var workInProgressRoot = getWorkInProgressRoot();
7415+
var workInProgressRootRenderLanes = getWorkInProgressRootRenderLanes();
74177416
var nextLanes = getNextLanes(
74187417
root,
74197418
root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes
@@ -7422,10 +7421,8 @@ function flushSyncWorkAcrossRoots_impl(onlyLegacy) {
74227421
if (includesSyncLane(nextLanes)) {
74237422
// This root has pending sync work. Flush it now.
74247423
try {
7425-
// TODO: Pass nextLanes as an argument instead of computing it again
7426-
// inside performSyncWorkOnRoot.
74277424
didPerformSomeWork = true;
7428-
performSyncWorkOnRoot(root);
7425+
performSyncWorkOnRoot(root, nextLanes);
74297426
} catch (error) {
74307427
// Collect errors so we can rethrow them at the end
74317428
if (errors === null) {
@@ -24629,25 +24626,28 @@ function markRootSuspended(root, suspendedLanes) {
2462924626
} // This is the entry point for synchronous tasks that don't go
2463024627
// through Scheduler
2463124628

24632-
function performSyncWorkOnRoot(root) {
24633-
{
24634-
syncNestedUpdateFlag();
24635-
}
24636-
24629+
function performSyncWorkOnRoot(root, lanes) {
2463724630
if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
2463824631
throw new Error("Should not already be working.");
2463924632
}
2464024633

24641-
flushPassiveEffects(); // TODO: This was already computed in the caller. Pass it as an argument.
24642-
24643-
var lanes = getNextLanes(root, NoLanes);
24634+
var didFlushPassiveEffects = flushPassiveEffects();
2464424635

24645-
if (!includesSyncLane(lanes)) {
24646-
// There's no remaining sync work left.
24636+
if (didFlushPassiveEffects) {
24637+
// If passive effects were flushed, exit to the outer work loop in the root
24638+
// scheduler, so we can recompute the priority.
24639+
// TODO: We don't actually need this `ensureRootIsScheduled` call because
24640+
// this path is only reachable if the root is already part of the schedule.
24641+
// I'm including it only for consistency with the other exit points from
24642+
// this function. Can address in a subsequent refactor.
2464724643
ensureRootIsScheduled(root);
2464824644
return null;
2464924645
}
2465024646

24647+
{
24648+
syncNestedUpdateFlag();
24649+
}
24650+
2465124651
var exitStatus = renderRootSync(root, lanes);
2465224652

2465324653
if (root.tag !== LegacyRoot && exitStatus === RootErrored) {

compiled/facebook-www/ReactART-dev.modern.js

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ function _assertThisInitialized(self) {
6969
return self;
7070
}
7171

72-
var ReactVersion = "18.3.0-www-modern-b17bda5a";
72+
var ReactVersion = "18.3.0-www-modern-87c144fd";
7373

7474
var LegacyRoot = 0;
7575
var ConcurrentRoot = 1;
@@ -7154,10 +7154,7 @@ function flushSyncWorkAcrossRoots_impl(onlyLegacy) {
71547154
if (!mightHavePendingSyncWork) {
71557155
// Fast path. There's no sync work to do.
71567156
return;
7157-
}
7158-
7159-
var workInProgressRoot = getWorkInProgressRoot();
7160-
var workInProgressRootRenderLanes = getWorkInProgressRootRenderLanes(); // There may or may not be synchronous work scheduled. Let's check.
7157+
} // There may or may not be synchronous work scheduled. Let's check.
71617158

71627159
var didPerformSomeWork;
71637160
var errors = null;
@@ -7170,6 +7167,8 @@ function flushSyncWorkAcrossRoots_impl(onlyLegacy) {
71707167
while (root !== null) {
71717168
if (onlyLegacy && root.tag !== LegacyRoot);
71727169
else {
7170+
var workInProgressRoot = getWorkInProgressRoot();
7171+
var workInProgressRootRenderLanes = getWorkInProgressRootRenderLanes();
71737172
var nextLanes = getNextLanes(
71747173
root,
71757174
root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes
@@ -7178,10 +7177,8 @@ function flushSyncWorkAcrossRoots_impl(onlyLegacy) {
71787177
if (includesSyncLane(nextLanes)) {
71797178
// This root has pending sync work. Flush it now.
71807179
try {
7181-
// TODO: Pass nextLanes as an argument instead of computing it again
7182-
// inside performSyncWorkOnRoot.
71837180
didPerformSomeWork = true;
7184-
performSyncWorkOnRoot(root);
7181+
performSyncWorkOnRoot(root, nextLanes);
71857182
} catch (error) {
71867183
// Collect errors so we can rethrow them at the end
71877184
if (errors === null) {
@@ -24294,25 +24291,28 @@ function markRootSuspended(root, suspendedLanes) {
2429424291
} // This is the entry point for synchronous tasks that don't go
2429524292
// through Scheduler
2429624293

24297-
function performSyncWorkOnRoot(root) {
24298-
{
24299-
syncNestedUpdateFlag();
24300-
}
24301-
24294+
function performSyncWorkOnRoot(root, lanes) {
2430224295
if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
2430324296
throw new Error("Should not already be working.");
2430424297
}
2430524298

24306-
flushPassiveEffects(); // TODO: This was already computed in the caller. Pass it as an argument.
24307-
24308-
var lanes = getNextLanes(root, NoLanes);
24299+
var didFlushPassiveEffects = flushPassiveEffects();
2430924300

24310-
if (!includesSyncLane(lanes)) {
24311-
// There's no remaining sync work left.
24301+
if (didFlushPassiveEffects) {
24302+
// If passive effects were flushed, exit to the outer work loop in the root
24303+
// scheduler, so we can recompute the priority.
24304+
// TODO: We don't actually need this `ensureRootIsScheduled` call because
24305+
// this path is only reachable if the root is already part of the schedule.
24306+
// I'm including it only for consistency with the other exit points from
24307+
// this function. Can address in a subsequent refactor.
2431224308
ensureRootIsScheduled(root);
2431324309
return null;
2431424310
}
2431524311

24312+
{
24313+
syncNestedUpdateFlag();
24314+
}
24315+
2431624316
var exitStatus = renderRootSync(root, lanes);
2431724317

2431824318
if (root.tag !== LegacyRoot && exitStatus === RootErrored) {

0 commit comments

Comments
 (0)