Skip to content

Commit

Permalink
Refactor Update Queues to Fix Rebasing Bug
Browse files Browse the repository at this point in the history 
Fixes a bug related to rebasing updates. Once an update has committed,
it should never un-commit, even if interrupted by a higher priority
update. The fix includes a refactor of how update queues work.

This commit is a combination of two PRs:

- facebook#17483 by @sebmarkbage refactors the hook update queue
- facebook#17510 by @acdlite refactors the class and root update queue

Landing one without the other would cause state updates to sometimes be
inconsistent across components, so I've combined them into a single
commit in case they need to be reverted.

Co-authored-by: Sebastian Markbåge <sema@fb.com>
Co-authored-by: Andrew Clark <git@andrewclark.io>
  • Loading branch information
@acdlite @sebmarkbage
acdlite and sebmarkbage committed Dec 9, 2019
1 parent e039e69 commit 66066ea
Show file tree
Hide file tree
Showing 5 changed files with 479 additions and 344 deletions.
35 changes: 24 additions & 11 deletions packages/react-noop-renderer/src/createReactNoop.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1142,20 +1142,33 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {

function logUpdateQueue(updateQueue: UpdateQueue<mixed>, depth) {
log(' '.repeat(depth + 1) + 'QUEUED UPDATES');
const firstUpdate = updateQueue.firstUpdate;
if (!firstUpdate) {
const last = updateQueue.baseQueue;
if (last === null) {
return;
}
const first = last.next;
let update = first;
if (update !== null) {
do {
log(
' '.repeat(depth + 1) + '~',
'[' + update.expirationTime + ']',
);
} while (update !== null && update !== first);
}

log(
' '.repeat(depth + 1) + '~',
'[' + firstUpdate.expirationTime + ']',
);
while (firstUpdate.next) {
log(
' '.repeat(depth + 1) + '~',
'[' + firstUpdate.expirationTime + ']',
);
const lastPending = updateQueue.shared.pending;
if (lastPending !== null) {
const firstPending = lastPending.next;
let pendingUpdate = firstPending;
if (pendingUpdate !== null) {
do {
log(
' '.repeat(depth + 1) + '~',
'[' + pendingUpdate.expirationTime + ']',
);
} while (pendingUpdate !== null && pendingUpdate !== firstPending);
}
}
}

Expand Down
119 changes: 70 additions & 49 deletions packages/react-reconciler/src/ReactFiberHooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';

import ReactSharedInternals from 'shared/ReactSharedInternals';

import {NoWork} from './ReactFiberExpirationTime';
import {NoWork, Sync} from './ReactFiberExpirationTime';
import {readContext} from './ReactFiberNewContext';
import {createResponderListener} from './ReactFiberEvents';
import {
Expand Down Expand Up @@ -108,13 +108,13 @@ type Update<S, A> = {
action: A,
eagerReducer: ((S, A) => S) | null,
eagerState: S | null,
next: Update<S, A> | null,
next: Update<S, A>,

priority?: ReactPriorityLevel,
};

type UpdateQueue<S, A> = {
last: Update<S, A> | null,
pending: Update<S, A> | null,
dispatch: (A => mixed) | null,
lastRenderedReducer: ((S, A) => S) | null,
lastRenderedState: S | null,
Expand Down Expand Up @@ -144,7 +144,7 @@ export type Hook = {
memoizedState: any,

baseState: any,
baseUpdate: Update<any, any> | null,
baseQueue: Update<any, any> | null,
queue: UpdateQueue<any, any> | null,

next: Hook | null,
Expand Down Expand Up @@ -544,8 +544,8 @@ function mountWorkInProgressHook(): Hook {
memoizedState: null,

baseState: null,
baseQueue: null,
queue: null,
baseUpdate: null,

next: null,
};
Expand Down Expand Up @@ -604,8 +604,8 @@ function updateWorkInProgressHook(): Hook {
memoizedState: currentHook.memoizedState,

baseState: currentHook.baseState,
baseQueue: currentHook.baseQueue,
queue: currentHook.queue,
baseUpdate: currentHook.baseUpdate,

next: null,
};
Expand Down Expand Up @@ -645,7 +645,7 @@ function mountReducer<S, I, A>(
}
hook.memoizedState = hook.baseState = initialState;
const queue = (hook.queue = {
last: null,
pending: null,
dispatch: null,
lastRenderedReducer: reducer,
lastRenderedState: (initialState: any),
Expand Down Expand Up @@ -703,7 +703,7 @@ function updateReducer<S, I, A>(
// the base state unless the queue is empty.
// TODO: Not sure if this is the desired semantics, but it's what we
// do for gDSFP. I can't remember why.
if (hook.baseUpdate === queue.last) {
if (hook.baseQueue === null) {
hook.baseState = newState;
}

Expand All @@ -715,42 +715,55 @@ function updateReducer<S, I, A>(
return [hook.memoizedState, dispatch];
}

// The last update in the entire queue
const last = queue.last;
// The last update that is part of the base state.
const baseUpdate = hook.baseUpdate;
const baseState = hook.baseState;
// Find the first unprocessed update.
let first;
if (baseUpdate !== null) {
if (last !== null) {
// For the first update, the queue is a circular linked list where
// `queue.last.next = queue.first`. Once the first update commits, and
// the `baseUpdate` is no longer empty, we can unravel the list.
last.next = null;
const current: Hook = (currentHook: any);

// The last rebase update that is NOT part of the base state.
let baseQueue = current.baseQueue;

// The last pending update that hasn't been processed yet.
let pendingQueue = queue.pending;
if (pendingQueue !== null) {
// We have new updates that haven't been processed yet.
// We'll add them to the base queue.
if (baseQueue !== null) {
// Merge the pending queue and the base queue.
let baseFirst = baseQueue.next;
let pendingFirst = pendingQueue.next;
baseQueue.next = pendingFirst;
pendingQueue.next = baseFirst;
}
first = baseUpdate.next;
} else {
first = last !== null ? last.next : null;
current.baseQueue = baseQueue = pendingQueue;
queue.pending = null;
}
if (first !== null) {
let newState = baseState;

if (baseQueue !== null) {
// We have a queue to process.
let first = baseQueue.next;
let newState = current.baseState;

let newBaseState = null;
let newBaseUpdate = null;
let prevUpdate = baseUpdate;
let newBaseQueueFirst = null;
let newBaseQueueLast = null;
let update = first;
let didSkip = false;
do {
const updateExpirationTime = update.expirationTime;
if (updateExpirationTime < renderExpirationTime) {
// Priority is insufficient. Skip this update. If this is the first
// skipped update, the previous update/state is the new base
// update/state.
if (!didSkip) {
didSkip = true;
newBaseUpdate = prevUpdate;
const clone: Update<S, A> = {
expirationTime: update.expirationTime,
suspenseConfig: update.suspenseConfig,
action: update.action,
eagerReducer: update.eagerReducer,
eagerState: update.eagerState,
next: (null: any),
};
if (newBaseQueueLast === null) {
newBaseQueueFirst = newBaseQueueLast = clone;
newBaseState = newState;
} else {
newBaseQueueLast = newBaseQueueLast.next = clone;
}
// Update the remaining priority in the queue.
if (updateExpirationTime > currentlyRenderingFiber.expirationTime) {
Expand All @@ -760,6 +773,18 @@ function updateReducer<S, I, A>(
} else {
// This update does have sufficient priority.

if (newBaseQueueLast !== null) {
const clone: Update<S, A> = {
expirationTime: Sync, // This update is going to be committed so we never want uncommit it.
suspenseConfig: update.suspenseConfig,
action: update.action,
eagerReducer: update.eagerReducer,
eagerState: update.eagerState,
next: (null: any),
};
newBaseQueueLast = newBaseQueueLast.next = clone;
}

// Mark the event time of this update as relevant to this render pass.
// TODO: This should ideally use the true event time of this update rather than
// its priority which is a derived and not reverseable value.
Expand All @@ -781,13 +806,13 @@ function updateReducer<S, I, A>(
newState = reducer(newState, action);
}
}
prevUpdate = update;
update = update.next;
} while (update !== null && update !== first);

if (!didSkip) {
newBaseUpdate = prevUpdate;
if (newBaseQueueLast === null) {
newBaseState = newState;
} else {
newBaseQueueLast.next = (newBaseQueueFirst: any);
}

// Mark that the fiber performed work, but only if the new state is
Expand All @@ -797,8 +822,8 @@ function updateReducer<S, I, A>(
}

hook.memoizedState = newState;
hook.baseUpdate = newBaseUpdate;
hook.baseState = newBaseState;
hook.baseQueue = newBaseQueueLast;

queue.lastRenderedState = newState;
}
Expand All @@ -816,7 +841,7 @@ function mountState<S>(
}
hook.memoizedState = hook.baseState = initialState;
const queue = (hook.queue = {
last: null,
pending: null,
dispatch: null,
lastRenderedReducer: basicStateReducer,
lastRenderedState: (initialState: any),
Expand Down Expand Up @@ -1233,7 +1258,7 @@ function dispatchAction<S, A>(
action,
eagerReducer: null,
eagerState: null,
next: null,
next: (null: any),
};
if (__DEV__) {
update.priority = getCurrentPriorityLevel();
Expand Down Expand Up @@ -1267,27 +1292,23 @@ function dispatchAction<S, A>(
action,
eagerReducer: null,
eagerState: null,
next: null,
next: (null: any),
};

if (__DEV__) {
update.priority = getCurrentPriorityLevel();
}

// Append the update to the end of the list.
const last = queue.last;
if (last === null) {
const pending = queue.pending;
if (pending === null) {
// This is the first update. Create a circular list.
update.next = update;
} else {
const first = last.next;
if (first !== null) {
// Still circular.
update.next = first;
}
last.next = update;
update.next = pending.next;
pending.next = update;
}
queue.last = update;
queue.pending = update;

if (
fiber.expirationTime === NoWork &&
Expand Down
19 changes: 9 additions & 10 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';
import type {Interaction} from 'scheduler/src/Tracing';
import type {SuspenseConfig} from './ReactFiberSuspenseConfig';
import type {SuspenseState} from './ReactFiberSuspenseComponent';
import type {Hook} from './ReactFiberHooks';

import {
warnAboutDeprecatedLifecycles,
Expand Down Expand Up @@ -2859,7 +2860,7 @@ export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) {
// has triggered any high priority updates
const updateQueue = current.updateQueue;
if (updateQueue !== null) {
let update = updateQueue.firstUpdate;
let update = updateQueue.baseQueue;
while (update !== null) {
const priorityLevel = update.priority;
if (
Expand All @@ -2883,12 +2884,11 @@ export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) {
break;
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent:
if (
workInProgressNode.memoizedState !== null &&
workInProgressNode.memoizedState.baseUpdate !== null
) {
let update = workInProgressNode.memoizedState.baseUpdate;
case SimpleMemoComponent: {
let firstHook: null | Hook = current.memoizedState;
// TODO: This just checks the first Hook. Isn't it suppose to check all Hooks?
if (firstHook !== null && firstHook.baseQueue !== null) {
let update = firstHook.baseQueue;
// Loop through the functional component's memoized state to see whether
// the component has triggered any high pri updates
while (update !== null) {
Expand All @@ -2908,15 +2908,14 @@ export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) {
}
break;
}
if (
update.next === workInProgressNode.memoizedState.baseUpdate
) {
if (update.next === firstHook.baseQueue) {
break;
}
update = update.next;
}
}
break;
}
default:
break;
}
Expand Down
Loading

0 comments on commit 66066ea

Please sign in to comment.