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

Hooks Refactor Update Queue by Cloning Rebased Updates #17483

Closed
wants to merge 2 commits into from
Closed

Hooks Refactor Update Queue by Cloning Rebased Updates #17483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 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
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
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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I love this trick

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 believe we could use this same trick for expirationTimes. By storing it temporarily on a shared stateful object and then transferring it to “current”. That way we don’t need the alternate model when we backtrack and can just backtrack along on a shared object path.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I used a similar trick for the eager bailout optimization. Where I reset current.expirationTime to NoWork after a bailout. Also in my useTransition PR to clear a pending transition. Such a nice strategy.

}
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
17 changes: 8 additions & 9 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 @@ -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
64 changes: 64 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js
View file
Open in desktop
Expand Up @@ -653,4 +653,68 @@ describe('ReactIncrementalUpdates', () => {
expect(Scheduler).toFlushAndYield(['Commit: goodbye']);
});
});

it('when rebasing, does not exclude updates that were already committed, regardless of priority', async () => {
const {useState, useLayoutEffect} = React;

let pushToLog;
function App() {
const [log, setLog] = useState('');
pushToLog = msg => {
setLog(prevLog => prevLog + msg);
};

useLayoutEffect(
() => {
Scheduler.unstable_yieldValue('Committed: ' + log);
if (log === 'B') {
// Right after B commits, schedule additional updates.
Scheduler.unstable_runWithPriority(
Scheduler.unstable_UserBlockingPriority,
() => {
pushToLog('C');
},
);
setLog(prevLog => prevLog + 'D');
}
},
[log],
);

return log;
}

const root = ReactNoop.createRoot();
await ReactNoop.act(async () => {
root.render(<App />);
});
expect(Scheduler).toHaveYielded(['Committed: ']);
expect(root).toMatchRenderedOutput('');

await ReactNoop.act(async () => {
pushToLog('A');
Scheduler.unstable_runWithPriority(
Scheduler.unstable_UserBlockingPriority,
() => {
pushToLog('B');
},
);
});
expect(Scheduler).toHaveYielded([
// A and B are pending. B is higher priority, so we'll render that first.
'Committed: B',
// Because A comes first in the queue, we're now in rebase mode. B must
// be rebased on top of A. Also, in a layout effect, we received two new
// updates: C and D. C is user-blocking and D is synchronous.
//
// First render the synchronous update. What we're testing here is that
// B *is not dropped* even though it has lower than sync priority. That's
// because we already committed it. However, this render should not
// include C, because that update wasn't already committed.
'Committed: BD',
'Committed: BCD',
'Committed: ABCD',
]);
expect(root).toMatchRenderedOutput('ABCD');
});
});