Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor legacy update queue #17510

Closed
wants to merge 4 commits into from
Closed

Refactor legacy update queue #17510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
474e99d
Failing test: Updates "un-committed" when rebasing
acdlite Nov 21, 2019
89ca3cf
Refactor Hooks update queue
sebmarkbage Nov 28, 2019
aeee2bf
Add regression test for update queue bug
acdlite Dec 9, 2019
99444a2
Refactor legacy update queue
acdlite Dec 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 24 additions & 11 deletions packages/react-noop-renderer/src/createReactNoop.js
View file
Open in desktop
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
Open in desktop
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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this about, we need this for it to stay circular, no? Maybe we're missing a test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All I did was remove the first !== null check. Which is unnecessary because update.next is never null.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the diff makes it look like I deleted the assignment but really all I deleted was the type check that surrounds it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, to rephrase, it's not really a type check that I deleted. In the old implementation, the queue was only sometimes circular. It would eventually "unravel" to linear so that the old nodes could be garbage collected. But in the new implementation, the pending queue is always circular. So you don't need to check for if it's circular or not.

}
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
Open in desktop
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