Skip to content

Commit

Permalink
Use lastPendingTime instead of Idle
Browse files Browse the repository at this point in the history
We don't currently have an mechanism to check if there are lower
priority updates in a subtree, but we can check if there are any in the
whole root. This still isn't perfect but it's better than using Idle,
which frequently leads to redundant re-renders.

When we refactor `expirationTime` to be a bitmask, this will no longer
be necessary because we'll know exactly which "task bits" remain.
  • Loading branch information
acdlite committed Mar 26, 2020
1 parent 9ebadfe commit 4ffc49a
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 52 deletions.
24 changes: 9 additions & 15 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Expand Up @@ -181,6 +181,7 @@ import {
scheduleUpdateOnFiber,
renderDidSuspendDelayIfPossible,
markUnprocessedUpdateTime,
getWorkInProgressRoot,
} from './ReactFiberWorkLoop';

const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;
Expand Down Expand Up @@ -1605,23 +1606,16 @@ function getRemainingWorkInPrimaryTree(
if ((workInProgress.mode & BlockingMode) !== NoMode) {
// The highest priority remaining work is part of this render. Since we only
// keep track of the highest level, we don't know if there's a lower
// priority level scheduled. As a compromise, we'll render at a really low
// priority that includes all the updates.
// priority level scheduled. As a compromise, we'll render at the lowest
// known level in the entire tree, since that will include everything.
// TODO: If expirationTime were a bitmask where each bit represents a
// separate task thread, this would be: currentChildBits & ~renderBits
// TODO: We used to track the lowest pending level for the whole root, but
// we removed it to simplify the implementation. It might be worth adding
// it back for this use case, to avoid redundant passes.
if (renderExpirationTime > ContinuousHydration) {
// First we try ContinuousHydration, so that we don't get grouped
// with Idle.
return ContinuousHydration;
} else if (renderExpirationTime > Idle) {
// Then we'll try Idle.
return Idle;
} else {
// Already at lowest possible update level.
return NoWork;
const root = getWorkInProgressRoot();
if (root !== null) {
const lastPendingTime = root.lastPendingTime;
if (lastPendingTime < renderExpirationTime) {
return lastPendingTime;
}
}
}
// In legacy mode, there's no work left.
Expand Down
Expand Up @@ -734,22 +734,12 @@ describe('ReactHooksWithNoopRenderer', () => {
root.render(<Foo signal={false} />);
setLabel('B');
});
expect(Scheduler).toHaveYielded([
'Suspend!',
// TODO: This second attempt is because React isn't sure if there's
// another update at a lower priority. Ideally we wouldn't need it.
'Suspend!',
]);
expect(Scheduler).toHaveYielded(['Suspend!']);
expect(root).toMatchRenderedOutput(<span prop="A:0" />);

// Rendering again should suspend again.
root.render(<Foo signal={false} />);
expect(Scheduler).toFlushAndYield([
'Suspend!',
// TODO: This second attempt is because React isn't sure if there's
// another update at a lower priority. Ideally we wouldn't need it.
'Suspend!',
]);
expect(Scheduler).toFlushAndYield(['Suspend!']);

// Flip the signal back to "cancel" the update. However, the update to
// label should still proceed. It shouldn't have been dropped.
Expand Down
Expand Up @@ -956,14 +956,7 @@ describe('ReactSuspense', () => {

// Update that suspends
instance.setState({step: 2});
expect(Scheduler).toFlushAndYield([
'Suspend! [Step: 2]',
'Loading...',
// TODO: This second attempt is because React isn't sure if there's
// another update at a lower priority. Ideally we wouldn't need it.
'Suspend! [Step: 2]',
'Loading...',
]);
expect(Scheduler).toFlushAndYield(['Suspend! [Step: 2]', 'Loading...']);
jest.advanceTimersByTime(500);
expect(root).toMatchRenderedOutput('Loading...');

Expand Down
Expand Up @@ -3091,15 +3091,6 @@ describe('ReactSuspenseWithNoopRenderer', () => {
// First we attempt the high pri update. It suspends.
'Suspend! [B]',
'Loading...',

// Then retry at a lower priority level.
// NOTE: The current implementation doesn't know which level to attempt,
// so it picks ContinuousHydration, which is one level higher than Idle.
// Since it doesn't include the Idle update, it will suspend again and
// reschedule to try at Idle. If we refactor expiration time to be a
// bitmask, we shouldn't need this heuristic.
'Suspend! [B]',
'Loading...',
]);

// Commit the placeholder to unblock the Idle update.
Expand Down Expand Up @@ -3166,20 +3157,13 @@ describe('ReactSuspenseWithNoopRenderer', () => {
await ReactNoop.act(async () => {
setText('C');
});
expect(Scheduler).toHaveYielded([
'Suspend! [C]',
'Loading...',

'Suspend! [C]',
'Loading...',
]);
expect(Scheduler).toHaveYielded(['Suspend! [C]', 'Loading...']);

// Commit. This will insert a fragment fiber to wrap around the component
// that triggered the update.
await ReactNoop.act(async () => {
await advanceTimers(250);
});
expect(Scheduler).toHaveYielded(['Suspend! [C]']);
// The fragment fiber is part of the current tree, but the setState return
// path still follows the alternate path. That means the fragment fiber is
// not part of the return path.
Expand Down

0 comments on commit 4ffc49a

Please sign in to comment.