Skip to content

Commit

Permalink
Idle updates should not be blocked by hidden work (#16871)
Browse files Browse the repository at this point in the history
* Idle updates should not be blocked by hidden work

Use the special `Idle` expiration time for updates that are triggered at
Scheduler's `IdlePriority`, instead of `Never`.

The key difference between Idle and Never¹ is that Never work can be
committed in an inconsistent state without tearing the UI. The main
example is offscreen content, like a hidden subtree.

¹ "Never" isn't the best name. I originally called it that because it
"never" expires, but neither does Idle. Since it's mostly used for
offscreen subtrees, we could call it "Offscreen." However, it's also
used for dehydrated Suspense boundaries, which are inconsistent in the
sense that they haven't finished yet, but aren't visibly inconsistent
because the server rendered HTML matches what the hydrated tree would
look like.

* Reset as early as possible using local variable

* Updates in a hidden effect should be Idle

I had made them Never to avoid an extra render when a hidden effect
updates the hidden component -- if they are Idle, we have to render once
at Idle, which bails out on the hidden subtree, then again at Never to
actually process the update -- but the problem of needing an extra
render pass to bail out hidden updates already exists and we should fix
that properly instead of adding yet another special case.
  • Loading branch information
acdlite authored Sep 24, 2019
1 parent c5e7190 commit 8b580a8
Show file tree
Hide file tree
Showing 5 changed files with 104 additions and 18 deletions.
13 changes: 10 additions & 3 deletions packages/react-reconciler/src/ReactFiberExpirationTime.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,16 @@ import {
export type ExpirationTime = number;

export const NoWork = 0;
// TODO: Think of a better name for Never.
// TODO: Think of a better name for Never. The key difference with Idle is that
// Never work can be committed in an inconsistent state without tearing the UI.
// The main example is offscreen content, like a hidden subtree. So one possible
// name is Offscreen. However, it also includes dehydrated Suspense boundaries,
// which are inconsistent in the sense that they haven't finished yet, but
// aren't visibly inconsistent because the server rendered HTML matches what the
// hydrated tree would look like.
export const Never = 1;
// TODO: Use the Idle expiration time for idle state updates
// Idle is slightly higher priority than Never. It must completely finish in
// order to be consistent.
export const Idle = 2;
export const Sync = MAX_SIGNED_31_BIT_INT;
export const Batched = Sync - 1;
Expand Down Expand Up @@ -115,7 +122,7 @@ export function inferPriorityFromExpirationTime(
if (expirationTime === Sync) {
return ImmediatePriority;
}
if (expirationTime === Never) {
if (expirationTime === Never || expirationTime === Idle) {
return IdlePriority;
}
const msUntil =
Expand Down
29 changes: 16 additions & 13 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,7 @@ export function computeExpirationForFiber(

if ((executionContext & RenderContext) !== NoContext) {
// Use whatever time we're already rendering
// TODO: Should there be a way to opt out, like with `runWithPriority`?
return renderExpirationTime;
}

Expand All @@ -347,7 +348,7 @@ export function computeExpirationForFiber(
expirationTime = computeAsyncExpiration(currentTime);
break;
case IdlePriority:
expirationTime = Never;
expirationTime = Idle;
break;
default:
invariant(false, 'Expected a valid priority level');
Expand Down Expand Up @@ -1406,14 +1407,14 @@ export function markRenderEventTimeAndConfig(
): void {
if (
expirationTime < workInProgressRootLatestProcessedExpirationTime &&
expirationTime > Never
expirationTime > Idle
) {
workInProgressRootLatestProcessedExpirationTime = expirationTime;
}
if (suspenseConfig !== null) {
if (
expirationTime < workInProgressRootLatestSuspenseTimeout &&
expirationTime > Never
expirationTime > Idle
) {
workInProgressRootLatestSuspenseTimeout = expirationTime;
// Most of the time we only have one config and getting wrong is not bad.
Expand Down Expand Up @@ -2203,24 +2204,25 @@ function commitLayoutEffects(
}

export function flushPassiveEffects() {
if (pendingPassiveEffectsRenderPriority !== NoPriority) {
const priorityLevel =
pendingPassiveEffectsRenderPriority > NormalPriority
? NormalPriority
: pendingPassiveEffectsRenderPriority;
pendingPassiveEffectsRenderPriority = NoPriority;
return runWithPriority(priorityLevel, flushPassiveEffectsImpl);
}
}

function flushPassiveEffectsImpl() {
if (rootWithPendingPassiveEffects === null) {
return false;
}
const root = rootWithPendingPassiveEffects;
const expirationTime = pendingPassiveEffectsExpirationTime;
const renderPriorityLevel = pendingPassiveEffectsRenderPriority;
rootWithPendingPassiveEffects = null;
pendingPassiveEffectsExpirationTime = NoWork;
pendingPassiveEffectsRenderPriority = NoPriority;
const priorityLevel =
renderPriorityLevel > NormalPriority ? NormalPriority : renderPriorityLevel;
return runWithPriority(
priorityLevel,
flushPassiveEffectsImpl.bind(null, root, expirationTime),
);
}

function flushPassiveEffectsImpl(root, expirationTime) {
invariant(
(executionContext & (RenderContext | CommitContext)) === NoContext,
'Cannot flush passive effects while already rendering.',
Expand Down Expand Up @@ -2263,6 +2265,7 @@ function flushPassiveEffectsImpl(root, expirationTime) {
}

executionContext = prevExecutionContext;

flushSyncCallbackQueue();

// If additional passive effects were scheduled, increment a counter. If this
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -352,4 +352,60 @@ describe('ReactSchedulerIntegration', () => {
Scheduler.unstable_flushUntilNextPaint();
expect(Scheduler).toHaveYielded(['A', 'B', 'C']);
});

it('idle updates are not blocked by offscreen work', async () => {
function Text({text}) {
Scheduler.unstable_yieldValue(text);
return text;
}

function App({label}) {
return (
<>
<Text text={`Visible: ` + label} />
<div hidden={true}>
<Text text={`Hidden: ` + label} />
</div>
</>
);
}

const root = ReactNoop.createRoot();
await ReactNoop.act(async () => {
root.render(<App label="A" />);

// Commit the visible content
expect(Scheduler).toFlushUntilNextPaint(['Visible: A']);
expect(root).toMatchRenderedOutput(
<>
Visible: A
<div hidden={true} />
</>,
);

// Before the hidden content has a chance to render, schedule an
// idle update
runWithPriority(IdlePriority, () => {
root.render(<App label="B" />);
});

// The next commit should only include the visible content
expect(Scheduler).toFlushUntilNextPaint(['Visible: B']);
expect(root).toMatchRenderedOutput(
<>
Visible: B
<div hidden={true} />
</>,
);
});

// The hidden content commits later
expect(Scheduler).toHaveYielded(['Hidden: B']);
expect(root).toMatchRenderedOutput(
<>
Visible: B
<div hidden={true}>Hidden: B</div>
</>,
);
});
});
14 changes: 12 additions & 2 deletions packages/react/src/__tests__/ReactDOMTracing-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,12 @@ describe('ReactDOMTracing', () => {
expect(
onInteractionScheduledWorkCompleted,
).toHaveBeenLastNotifiedOfInteraction(interaction);
expect(onRender).toHaveBeenCalledTimes(3);
// TODO: This is 4 instead of 3 because this update was scheduled at
// idle priority, and idle updates are slightly higher priority than
// offscreen work. So it takes two render passes to finish it. Profiler
// calls `onRender` for the first render even though everything
// bails out.
expect(onRender).toHaveBeenCalledTimes(4);
expect(onRender).toHaveLastRenderedWithInteractions(
new Set([interaction]),
);
Expand Down Expand Up @@ -281,7 +286,12 @@ describe('ReactDOMTracing', () => {
expect(
onInteractionScheduledWorkCompleted,
).toHaveBeenLastNotifiedOfInteraction(interaction);
expect(onRender).toHaveBeenCalledTimes(3);
// TODO: This is 4 instead of 3 because this update was scheduled at
// idle priority, and idle updates are slightly higher priority than
// offscreen work. So it takes two render passes to finish it. Profiler
// calls `onRender` for the first render even though everything
// bails out.
expect(onRender).toHaveBeenCalledTimes(4);
expect(onRender).toHaveLastRenderedWithInteractions(
new Set([interaction]),
);
Expand Down
10 changes: 10 additions & 0 deletions scripts/jest/matchers/schedulerTestMatchers.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,15 @@ function toFlushAndYieldThrough(Scheduler, expectedYields) {
});
}

function toFlushUntilNextPaint(Scheduler, expectedYields) {
assertYieldsWereCleared(Scheduler);
Scheduler.unstable_flushUntilNextPaint();
const actualYields = Scheduler.unstable_clearYields();
return captureAssertion(() => {
expect(actualYields).toEqual(expectedYields);
});
}

function toFlushWithoutYielding(Scheduler) {
return toFlushAndYield(Scheduler, []);
}
Expand Down Expand Up @@ -76,6 +85,7 @@ function toFlushAndThrow(Scheduler, ...rest) {
module.exports = {
toFlushAndYield,
toFlushAndYieldThrough,
toFlushUntilNextPaint,
toFlushWithoutYielding,
toFlushExpired,
toHaveYielded,
Expand Down

0 comments on commit 8b580a8

Please sign in to comment.