Skip to content

Commit

Permalink
Fix Suspense throttling mechanism (#26802)
Browse files Browse the repository at this point in the history 
The throttling mechanism for fallbacks should apply to both their
appearance _and_ disappearance.

This was mostly addressed by #26611. See that PR for additional context.

However, a flaw in the implementation is that we only update the the
timestamp used for throttling when the fallback initially appears. We
don't update it when the real content pops in. If lots of content in
separate Suspense trees loads around the same time, you can still get
jank.

The issue is fixed by updating the throttling timestamp whenever the
visibility of a fallback changes. Not just when it appears.

DiffTrain build for commit 4bfcd02.
  • Loading branch information
@acdlite
acdlite committed May 16, 2023
1 parent 3546b41 commit c488cce
Show file tree
Hide file tree
Showing 13 changed files with 427 additions and 394 deletions.
59 changes: 34 additions & 25 deletions ...ook-fbsource/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-dev.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @noflow
* @nolint
* @preventMunge
* @generated SignedSource<<ba2153287ac25e256482103ffe8356cc>>
* @generated SignedSource<<1559e1781d2b4f77a1b869cc6a27b095>>
*/

'use strict';
Expand Down Expand Up @@ -18377,20 +18377,29 @@ function commitMutationEffectsOnFiber(finishedWork, root, lanes) {

case SuspenseComponent: {
recursivelyTraverseMutationEffects(root, finishedWork);
commitReconciliationEffects(finishedWork);
commitReconciliationEffects(finishedWork); // TODO: We should mark a flag on the Suspense fiber itself, rather than
// relying on the Offscreen fiber having a flag also being marked. The
// reason is that this offscreen fiber might not be part of the work-in-
// progress tree! It could have been reused from a previous render. This
// doesn't lead to incorrect behavior because we don't rely on the flag
// check alone; we also compare the states explicitly below. But for
// modeling purposes, we _should_ be able to rely on the flag check alone.
// So this is a bit fragile.
//
// Also, all this logic could/should move to the passive phase so it
// doesn't block paint.

var offscreenFiber = finishedWork.child;

if (offscreenFiber.flags & Visibility) {
var newState = offscreenFiber.memoizedState;
var isHidden = newState !== null;

if (isHidden) {
var wasHidden =
offscreenFiber.alternate !== null &&
offscreenFiber.alternate.memoizedState !== null;
// Throttle the appearance and disappearance of Suspense fallbacks.
var isShowingFallback = finishedWork.memoizedState !== null;
var wasShowingFallback =
current !== null && current.memoizedState !== null;

if (!wasHidden) {
// TODO: Move to passive phase
{
if (isShowingFallback !== wasShowingFallback) {
// A fallback is either appearing or disappearing.
markCommitTimeOfFallback();
}
}
Expand Down Expand Up @@ -18421,20 +18430,18 @@ function commitMutationEffectsOnFiber(finishedWork, root, lanes) {
}
}

var _newState = finishedWork.memoizedState;

var _isHidden = _newState !== null;

var _wasHidden = current !== null && current.memoizedState !== null;
var newState = finishedWork.memoizedState;
var isHidden = newState !== null;
var wasHidden = current !== null && current.memoizedState !== null;

if (finishedWork.mode & ConcurrentMode) {
// Before committing the children, track on the stack whether this
// offscreen subtree was already hidden, so that we don't unmount the
// effects again.
var prevOffscreenSubtreeIsHidden = offscreenSubtreeIsHidden;
var prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
offscreenSubtreeIsHidden = prevOffscreenSubtreeIsHidden || _isHidden;
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || _wasHidden;
offscreenSubtreeIsHidden = prevOffscreenSubtreeIsHidden || isHidden;
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
recursivelyTraverseMutationEffects(root, finishedWork);
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
offscreenSubtreeIsHidden = prevOffscreenSubtreeIsHidden;
Expand All @@ -18455,21 +18462,21 @@ function commitMutationEffectsOnFiber(finishedWork, root, lanes) {
if (flags & Visibility) {
// Track the current state on the Offscreen instance so we can
// read it during an event
if (_isHidden) {
if (isHidden) {
offscreenInstance._visibility &= ~OffscreenVisible;
} else {
offscreenInstance._visibility |= OffscreenVisible;
}

if (_isHidden) {
if (isHidden) {
var isUpdate = current !== null;
var wasHiddenByAncestorOffscreen =
offscreenSubtreeIsHidden || offscreenSubtreeWasHidden; // Only trigger disapper layout effects if:
// - This is an update, not first mount.
// - This Offscreen was not hidden before.
// - Ancestor Offscreen was not hidden in previous commit.

if (isUpdate && !_wasHidden && !wasHiddenByAncestorOffscreen) {
if (isUpdate && !wasHidden && !wasHiddenByAncestorOffscreen) {
if ((finishedWork.mode & ConcurrentMode) !== NoMode) {
// Disappear the layout effects of all the children
recursivelyTraverseDisappearLayoutEffects(finishedWork);
Expand All @@ -18480,7 +18487,7 @@ function commitMutationEffectsOnFiber(finishedWork, root, lanes) {
if (!isOffscreenManual(finishedWork)) {
// TODO: This needs to run whenever there's an insertion or update
// inside a hidden Offscreen tree.
hideOrUnhideAllChildren(finishedWork, _isHidden);
hideOrUnhideAllChildren(finishedWork, isHidden);
}
} // TODO: Move to passive phase

Expand Down Expand Up @@ -20001,8 +20008,10 @@ var workInProgressRootPingedLanes = NoLanes; // Errors that are thrown during th
var workInProgressRootConcurrentErrors = null; // These are errors that we recovered from without surfacing them to the UI.
// We will log them once the tree commits.

var workInProgressRootRecoverableErrors = null; // The most recent time we committed a fallback. This lets us ensure a train
// model where we don't commit new loading states in too quick succession.
var workInProgressRootRecoverableErrors = null; // The most recent time we either committed a fallback, or when a fallback was
// filled in with the resolved UI. This lets us throttle the appearance of new
// content as it streams in, to minimize jank.
// TODO: Think of a better name for this variable?

var globalMostRecentFallbackTime = 0;
var FALLBACK_THROTTLE_MS = 500; // The absolute time for when we should start giving up on rendering
Expand Down Expand Up @@ -23921,7 +23930,7 @@ function createFiberRoot(
return root;
}

var ReactVersion = "18.3.0-canary-4cd706566-20230512";
var ReactVersion = "18.3.0-canary-4bfcd02b2-20230516";

// Might add PROFILE later.

Expand Down
108 changes: 51 additions & 57 deletions ...ok-fbsource/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-prod.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @noflow
* @nolint
* @preventMunge
* @generated SignedSource<<45020522021e7a01efc7d244b5d43b9f>>
* @generated SignedSource<<668f9e223a7ddfa69caac2f6094e95b5>>
*/

"use strict";
Expand Down Expand Up @@ -5617,11 +5617,9 @@ function commitMutationEffectsOnFiber(finishedWork, root) {
case 13:
recursivelyTraverseMutationEffects(root, finishedWork);
commitReconciliationEffects(finishedWork);
existingHiddenCallbacks = finishedWork.child;
existingHiddenCallbacks.flags & 8192 &&
null !== existingHiddenCallbacks.memoizedState &&
(null === existingHiddenCallbacks.alternate ||
null === existingHiddenCallbacks.alternate.memoizedState) &&
finishedWork.child.flags & 8192 &&
(null !== finishedWork.memoizedState) !==
(null !== current && null !== current.memoizedState) &&
(globalMostRecentFallbackTime = now());
flags & 4 &&
((flags = finishedWork.updateQueue),
Expand All @@ -5634,14 +5632,13 @@ function commitMutationEffectsOnFiber(finishedWork, root) {
null !== current &&
safelyDetachRef(current, current.return);
existingHiddenCallbacks = null !== finishedWork.memoizedState;
var wasHidden$99 = null !== current && null !== current.memoizedState;
var wasHidden = null !== current && null !== current.memoizedState;
if (finishedWork.mode & 1) {
var prevOffscreenSubtreeIsHidden = offscreenSubtreeIsHidden,
prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
offscreenSubtreeIsHidden =
prevOffscreenSubtreeIsHidden || existingHiddenCallbacks;
offscreenSubtreeWasHidden =
prevOffscreenSubtreeWasHidden || wasHidden$99;
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
recursivelyTraverseMutationEffects(root, finishedWork);
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
offscreenSubtreeIsHidden = prevOffscreenSubtreeIsHidden;
Expand All @@ -5659,22 +5656,22 @@ function commitMutationEffectsOnFiber(finishedWork, root) {
existingHiddenCallbacks &&
((root = offscreenSubtreeIsHidden || offscreenSubtreeWasHidden),
null === current ||
wasHidden$99 ||
wasHidden ||
root ||
(0 !== (finishedWork.mode & 1) &&
recursivelyTraverseDisappearLayoutEffects(finishedWork))),
null === finishedWork.memoizedProps ||
"manual" !== finishedWork.memoizedProps.mode)
)
a: for (current = null, wasHidden$99 = finishedWork; ; ) {
if (5 === wasHidden$99.tag) {
a: for (current = null, wasHidden = finishedWork; ; ) {
if (5 === wasHidden.tag) {
if (null === current) {
current = wasHidden$99;
current = wasHidden;
try {
(type = wasHidden$99.stateNode),
(type = wasHidden.stateNode),
existingHiddenCallbacks
? (type.isHidden = !0)
: (wasHidden$99.stateNode.isHidden = !1);
: (wasHidden.stateNode.isHidden = !1);
} catch (error) {
captureCommitPhaseError(
finishedWork,
Expand All @@ -5683,10 +5680,10 @@ function commitMutationEffectsOnFiber(finishedWork, root) {
);
}
}
} else if (6 === wasHidden$99.tag) {
} else if (6 === wasHidden.tag) {
if (null === current)
try {
wasHidden$99.stateNode.isHidden = existingHiddenCallbacks
wasHidden.stateNode.isHidden = existingHiddenCallbacks
? !0
: !1;
} catch (error$85) {
Expand All @@ -5697,28 +5694,25 @@ function commitMutationEffectsOnFiber(finishedWork, root) {
);
}
} else if (
((22 !== wasHidden$99.tag && 23 !== wasHidden$99.tag) ||
null === wasHidden$99.memoizedState ||
wasHidden$99 === finishedWork) &&
null !== wasHidden$99.child
((22 !== wasHidden.tag && 23 !== wasHidden.tag) ||
null === wasHidden.memoizedState ||
wasHidden === finishedWork) &&
null !== wasHidden.child
) {
wasHidden$99.child.return = wasHidden$99;
wasHidden$99 = wasHidden$99.child;
wasHidden.child.return = wasHidden;
wasHidden = wasHidden.child;
continue;
}
if (wasHidden$99 === finishedWork) break a;
for (; null === wasHidden$99.sibling; ) {
if (
null === wasHidden$99.return ||
wasHidden$99.return === finishedWork
)
if (wasHidden === finishedWork) break a;
for (; null === wasHidden.sibling; ) {
if (null === wasHidden.return || wasHidden.return === finishedWork)
break a;
current === wasHidden$99 && (current = null);
wasHidden$99 = wasHidden$99.return;
current === wasHidden && (current = null);
wasHidden = wasHidden.return;
}
current === wasHidden$99 && (current = null);
wasHidden$99.sibling.return = wasHidden$99.return;
wasHidden$99 = wasHidden$99.sibling;
current === wasHidden && (current = null);
wasHidden.sibling.return = wasHidden.return;
wasHidden = wasHidden.sibling;
}
flags & 4 &&
((flags = finishedWork.updateQueue),
Expand Down Expand Up @@ -6507,16 +6501,16 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
didTimeout = renderRootSync(root, lanes);
if (2 === didTimeout) {
errorRetryLanes = lanes;
var errorRetryLanes$107 = getLanesToRetrySynchronouslyOnError(
var errorRetryLanes$104 = getLanesToRetrySynchronouslyOnError(
root,
errorRetryLanes
);
0 !== errorRetryLanes$107 &&
((lanes = errorRetryLanes$107),
0 !== errorRetryLanes$104 &&
((lanes = errorRetryLanes$104),
(didTimeout = recoverFromConcurrentError(
root,
errorRetryLanes,
errorRetryLanes$107
errorRetryLanes$104
)));
}
if (1 === didTimeout)
Expand Down Expand Up @@ -6821,8 +6815,8 @@ function renderRootSync(root, lanes) {
}
workLoopSync();
break;
} catch (thrownValue$109) {
handleThrow(root, thrownValue$109);
} catch (thrownValue$106) {
handleThrow(root, thrownValue$106);
}
while (1);
resetContextDependencies();
Expand Down Expand Up @@ -6929,8 +6923,8 @@ function renderRootConcurrent(root, lanes) {
}
workLoopConcurrent();
break;
} catch (thrownValue$111) {
handleThrow(root, thrownValue$111);
} catch (thrownValue$108) {
handleThrow(root, thrownValue$108);
}
while (1);
resetContextDependencies();
Expand Down Expand Up @@ -8618,19 +8612,19 @@ function wrapFiber(fiber) {
fiberToWrapper.set(fiber, wrapper));
return wrapper;
}
var devToolsConfig$jscomp$inline_1039 = {
var devToolsConfig$jscomp$inline_1036 = {
findFiberByHostInstance: function () {
throw Error("TestRenderer does not support findFiberByHostInstance()");
},
bundleType: 0,
version: "18.3.0-canary-4cd706566-20230512",
version: "18.3.0-canary-4bfcd02b2-20230516",
rendererPackageName: "react-test-renderer"
};
var internals$jscomp$inline_1238 = {
bundleType: devToolsConfig$jscomp$inline_1039.bundleType,
version: devToolsConfig$jscomp$inline_1039.version,
rendererPackageName: devToolsConfig$jscomp$inline_1039.rendererPackageName,
rendererConfig: devToolsConfig$jscomp$inline_1039.rendererConfig,
var internals$jscomp$inline_1235 = {
bundleType: devToolsConfig$jscomp$inline_1036.bundleType,
version: devToolsConfig$jscomp$inline_1036.version,
rendererPackageName: devToolsConfig$jscomp$inline_1036.rendererPackageName,
rendererConfig: devToolsConfig$jscomp$inline_1036.rendererConfig,
overrideHookState: null,
overrideHookStateDeletePath: null,
overrideHookStateRenamePath: null,
Expand All @@ -8647,26 +8641,26 @@ var internals$jscomp$inline_1238 = {
return null === fiber ? null : fiber.stateNode;
},
findFiberByHostInstance:
devToolsConfig$jscomp$inline_1039.findFiberByHostInstance ||
devToolsConfig$jscomp$inline_1036.findFiberByHostInstance ||
emptyFindFiberByHostInstance,
findHostInstancesForRefresh: null,
scheduleRefresh: null,
scheduleRoot: null,
setRefreshHandler: null,
getCurrentFiber: null,
reconcilerVersion: "18.3.0-canary-4cd706566-20230512"
reconcilerVersion: "18.3.0-canary-4bfcd02b2-20230516"
};
if ("undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__) {
var hook$jscomp$inline_1239 = __REACT_DEVTOOLS_GLOBAL_HOOK__;
var hook$jscomp$inline_1236 = __REACT_DEVTOOLS_GLOBAL_HOOK__;
if (
!hook$jscomp$inline_1239.isDisabled &&
hook$jscomp$inline_1239.supportsFiber
!hook$jscomp$inline_1236.isDisabled &&
hook$jscomp$inline_1236.supportsFiber
)
try {
(rendererID = hook$jscomp$inline_1239.inject(
internals$jscomp$inline_1238
(rendererID = hook$jscomp$inline_1236.inject(
internals$jscomp$inline_1235
)),
(injectedHook = hook$jscomp$inline_1239);
(injectedHook = hook$jscomp$inline_1236);
} catch (err) {}
}
exports._Scheduler = Scheduler;
Expand Down
Loading

0 comments on commit c488cce

Please sign in to comment.