Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 6638815

Browse files
josephsavonajosephsavona
authoredSep 27, 2021
Remove usereducer eager bailout (#22445)
* Fork dispatchAction for useState/useReducer * Remove eager bailout from forked dispatchReducerAction, update tests * Update eager reducer/state logic to handle state case only * sync reconciler fork * rename test * test cases from #15198 * comments on new test cases * comments on new test cases * test case from #21419 * minor tweak to test name to kick CI
1 parent 8131de1 commit 6638815

File tree

3 files changed

+408
-27
lines changed

3 files changed

+408
-27
lines changed
 

‎packages/react-reconciler/src/ReactFiberHooks.new.js

Lines changed: 124 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals;
125125
type Update<S, A> = {|
126126
lane: Lane,
127127
action: A,
128-
eagerReducer: ((S, A) => S) | null,
128+
hasEagerState: boolean,
129129
eagerState: S | null,
130130
next: Update<S, A>,
131131
|};
@@ -730,7 +730,7 @@ function mountReducer<S, I, A>(
730730
lastRenderedState: (initialState: any),
731731
};
732732
hook.queue = queue;
733-
const dispatch: Dispatch<A> = (queue.dispatch = (dispatchAction.bind(
733+
const dispatch: Dispatch<A> = (queue.dispatch = (dispatchReducerAction.bind(
734734
null,
735735
currentlyRenderingFiber,
736736
queue,
@@ -801,7 +801,7 @@ function updateReducer<S, I, A>(
801801
const clone: Update<S, A> = {
802802
lane: updateLane,
803803
action: update.action,
804-
eagerReducer: update.eagerReducer,
804+
hasEagerState: update.hasEagerState,
805805
eagerState: update.eagerState,
806806
next: (null: any),
807807
};
@@ -829,17 +829,17 @@ function updateReducer<S, I, A>(
829829
// this will never be skipped by the check above.
830830
lane: NoLane,
831831
action: update.action,
832-
eagerReducer: update.eagerReducer,
832+
hasEagerState: update.hasEagerState,
833833
eagerState: update.eagerState,
834834
next: (null: any),
835835
};
836836
newBaseQueueLast = newBaseQueueLast.next = clone;
837837
}
838838

839839
// Process this update.
840-
if (update.eagerReducer === reducer) {
841-
// If this update was processed eagerly, and its reducer matches the
842-
// current reducer, we can use the eagerly computed state.
840+
if (update.hasEagerState) {
841+
// If this update is a state update (not a reducer) and was processed eagerly,
842+
// we can use the eagerly computed state
843843
newState = ((update.eagerState: any): S);
844844
} else {
845845
const action = update.action;
@@ -1190,7 +1190,7 @@ function useMutableSource<Source, Snapshot>(
11901190
lastRenderedReducer: basicStateReducer,
11911191
lastRenderedState: snapshot,
11921192
};
1193-
newQueue.dispatch = setSnapshot = (dispatchAction.bind(
1193+
newQueue.dispatch = setSnapshot = (dispatchSetState.bind(
11941194
null,
11951195
currentlyRenderingFiber,
11961196
newQueue,
@@ -1481,7 +1481,7 @@ function mountState<S>(
14811481
hook.queue = queue;
14821482
const dispatch: Dispatch<
14831483
BasicStateAction<S>,
1484-
> = (queue.dispatch = (dispatchAction.bind(
1484+
> = (queue.dispatch = (dispatchSetState.bind(
14851485
null,
14861486
currentlyRenderingFiber,
14871487
queue,
@@ -2150,7 +2150,7 @@ function refreshCache<T>(fiber: Fiber, seedKey: ?() => T, seedValue: T) {
21502150
// TODO: Warn if unmounted?
21512151
}
21522152

2153-
function dispatchAction<S, A>(
2153+
function dispatchReducerAction<S, A>(
21542154
fiber: Fiber,
21552155
queue: UpdateQueue<S, A>,
21562156
action: A,
@@ -2171,7 +2171,119 @@ function dispatchAction<S, A>(
21712171
const update: Update<S, A> = {
21722172
lane,
21732173
action,
2174-
eagerReducer: null,
2174+
hasEagerState: false,
2175+
eagerState: null,
2176+
next: (null: any),
2177+
};
2178+
2179+
const alternate = fiber.alternate;
2180+
if (
2181+
fiber === currentlyRenderingFiber ||
2182+
(alternate !== null && alternate === currentlyRenderingFiber)
2183+
) {
2184+
// This is a render phase update. Stash it in a lazily-created map of
2185+
// queue -> linked list of updates. After this render pass, we'll restart
2186+
// and apply the stashed updates on top of the work-in-progress hook.
2187+
didScheduleRenderPhaseUpdateDuringThisPass = didScheduleRenderPhaseUpdate = true;
2188+
const pending = queue.pending;
2189+
if (pending === null) {
2190+
// This is the first update. Create a circular list.
2191+
update.next = update;
2192+
} else {
2193+
update.next = pending.next;
2194+
pending.next = update;
2195+
}
2196+
queue.pending = update;
2197+
} else {
2198+
if (isInterleavedUpdate(fiber, lane)) {
2199+
const interleaved = queue.interleaved;
2200+
if (interleaved === null) {
2201+
// This is the first update. Create a circular list.
2202+
update.next = update;
2203+
// At the end of the current render, this queue's interleaved updates will
2204+
// be transferred to the pending queue.
2205+
pushInterleavedQueue(queue);
2206+
} else {
2207+
update.next = interleaved.next;
2208+
interleaved.next = update;
2209+
}
2210+
queue.interleaved = update;
2211+
} else {
2212+
const pending = queue.pending;
2213+
if (pending === null) {
2214+
// This is the first update. Create a circular list.
2215+
update.next = update;
2216+
} else {
2217+
update.next = pending.next;
2218+
pending.next = update;
2219+
}
2220+
queue.pending = update;
2221+
}
2222+
2223+
if (__DEV__) {
2224+
// $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
2225+
if ('undefined' !== typeof jest) {
2226+
warnIfNotCurrentlyActingUpdatesInDev(fiber);
2227+
}
2228+
}
2229+
const root = scheduleUpdateOnFiber(fiber, lane, eventTime);
2230+
2231+
if (isTransitionLane(lane) && root !== null) {
2232+
let queueLanes = queue.lanes;
2233+
2234+
// If any entangled lanes are no longer pending on the root, then they
2235+
// must have finished. We can remove them from the shared queue, which
2236+
// represents a superset of the actually pending lanes. In some cases we
2237+
// may entangle more than we need to, but that's OK. In fact it's worse if
2238+
// we *don't* entangle when we should.
2239+
queueLanes = intersectLanes(queueLanes, root.pendingLanes);
2240+
2241+
// Entangle the new transition lane with the other transition lanes.
2242+
const newQueueLanes = mergeLanes(queueLanes, lane);
2243+
queue.lanes = newQueueLanes;
2244+
// Even if queue.lanes already include lane, we don't know for certain if
2245+
// the lane finished since the last time we entangled it. So we need to
2246+
// entangle it again, just to be sure.
2247+
markRootEntangled(root, newQueueLanes);
2248+
}
2249+
}
2250+
2251+
if (__DEV__) {
2252+
if (enableDebugTracing) {
2253+
if (fiber.mode & DebugTracingMode) {
2254+
const name = getComponentNameFromFiber(fiber) || 'Unknown';
2255+
logStateUpdateScheduled(name, lane, action);
2256+
}
2257+
}
2258+
}
2259+
2260+
if (enableSchedulingProfiler) {
2261+
markStateUpdateScheduled(fiber, lane);
2262+
}
2263+
}
2264+
2265+
function dispatchSetState<S, A>(
2266+
fiber: Fiber,
2267+
queue: UpdateQueue<S, A>,
2268+
action: A,
2269+
) {
2270+
if (__DEV__) {
2271+
if (typeof arguments[3] === 'function') {
2272+
console.error(
2273+
"State updates from the useState() and useReducer() Hooks don't support the " +
2274+
'second callback argument. To execute a side effect after ' +
2275+
'rendering, declare it in the component body with useEffect().',
2276+
);
2277+
}
2278+
}
2279+
2280+
const eventTime = requestEventTime();
2281+
const lane = requestUpdateLane(fiber);
2282+
2283+
const update: Update<S, A> = {
2284+
lane,
2285+
action,
2286+
hasEagerState: false,
21752287
eagerState: null,
21762288
next: (null: any),
21772289
};
@@ -2241,7 +2353,7 @@ function dispatchAction<S, A>(
22412353
// it, on the update object. If the reducer hasn't changed by the
22422354
// time we enter the render phase, then the eager state can be used
22432355
// without calling the reducer again.
2244-
update.eagerReducer = lastRenderedReducer;
2356+
update.hasEagerState = true;
22452357
update.eagerState = eagerState;
22462358
if (is(eagerState, currentState)) {
22472359
// Fast path. We can bail out without scheduling React to re-render.

‎packages/react-reconciler/src/ReactFiberHooks.old.js

Lines changed: 124 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals;
125125
type Update<S, A> = {|
126126
lane: Lane,
127127
action: A,
128-
eagerReducer: ((S, A) => S) | null,
128+
hasEagerState: boolean,
129129
eagerState: S | null,
130130
next: Update<S, A>,
131131
|};
@@ -730,7 +730,7 @@ function mountReducer<S, I, A>(
730730
lastRenderedState: (initialState: any),
731731
};
732732
hook.queue = queue;
733-
const dispatch: Dispatch<A> = (queue.dispatch = (dispatchAction.bind(
733+
const dispatch: Dispatch<A> = (queue.dispatch = (dispatchReducerAction.bind(
734734
null,
735735
currentlyRenderingFiber,
736736
queue,
@@ -801,7 +801,7 @@ function updateReducer<S, I, A>(
801801
const clone: Update<S, A> = {
802802
lane: updateLane,
803803
action: update.action,
804-
eagerReducer: update.eagerReducer,
804+
hasEagerState: update.hasEagerState,
805805
eagerState: update.eagerState,
806806
next: (null: any),
807807
};
@@ -829,17 +829,17 @@ function updateReducer<S, I, A>(
829829
// this will never be skipped by the check above.
830830
lane: NoLane,
831831
action: update.action,
832-
eagerReducer: update.eagerReducer,
832+
hasEagerState: update.hasEagerState,
833833
eagerState: update.eagerState,
834834
next: (null: any),
835835
};
836836
newBaseQueueLast = newBaseQueueLast.next = clone;
837837
}
838838

839839
// Process this update.
840-
if (update.eagerReducer === reducer) {
841-
// If this update was processed eagerly, and its reducer matches the
842-
// current reducer, we can use the eagerly computed state.
840+
if (update.hasEagerState) {
841+
// If this update is a state update (not a reducer) and was processed eagerly,
842+
// we can use the eagerly computed state
843843
newState = ((update.eagerState: any): S);
844844
} else {
845845
const action = update.action;
@@ -1190,7 +1190,7 @@ function useMutableSource<Source, Snapshot>(
11901190
lastRenderedReducer: basicStateReducer,
11911191
lastRenderedState: snapshot,
11921192
};
1193-
newQueue.dispatch = setSnapshot = (dispatchAction.bind(
1193+
newQueue.dispatch = setSnapshot = (dispatchSetState.bind(
11941194
null,
11951195
currentlyRenderingFiber,
11961196
newQueue,
@@ -1481,7 +1481,7 @@ function mountState<S>(
14811481
hook.queue = queue;
14821482
const dispatch: Dispatch<
14831483
BasicStateAction<S>,
1484-
> = (queue.dispatch = (dispatchAction.bind(
1484+
> = (queue.dispatch = (dispatchSetState.bind(
14851485
null,
14861486
currentlyRenderingFiber,
14871487
queue,
@@ -2150,7 +2150,7 @@ function refreshCache<T>(fiber: Fiber, seedKey: ?() => T, seedValue: T) {
21502150
// TODO: Warn if unmounted?
21512151
}
21522152

2153-
function dispatchAction<S, A>(
2153+
function dispatchReducerAction<S, A>(
21542154
fiber: Fiber,
21552155
queue: UpdateQueue<S, A>,
21562156
action: A,
@@ -2171,7 +2171,119 @@ function dispatchAction<S, A>(
21712171
const update: Update<S, A> = {
21722172
lane,
21732173
action,
2174-
eagerReducer: null,
2174+
hasEagerState: false,
2175+
eagerState: null,
2176+
next: (null: any),
2177+
};
2178+
2179+
const alternate = fiber.alternate;
2180+
if (
2181+
fiber === currentlyRenderingFiber ||
2182+
(alternate !== null && alternate === currentlyRenderingFiber)
2183+
) {
2184+
// This is a render phase update. Stash it in a lazily-created map of
2185+
// queue -> linked list of updates. After this render pass, we'll restart
2186+
// and apply the stashed updates on top of the work-in-progress hook.
2187+
didScheduleRenderPhaseUpdateDuringThisPass = didScheduleRenderPhaseUpdate = true;
2188+
const pending = queue.pending;
2189+
if (pending === null) {
2190+
// This is the first update. Create a circular list.
2191+
update.next = update;
2192+
} else {
2193+
update.next = pending.next;
2194+
pending.next = update;
2195+
}
2196+
queue.pending = update;
2197+
} else {
2198+
if (isInterleavedUpdate(fiber, lane)) {
2199+
const interleaved = queue.interleaved;
2200+
if (interleaved === null) {
2201+
// This is the first update. Create a circular list.
2202+
update.next = update;
2203+
// At the end of the current render, this queue's interleaved updates will
2204+
// be transferred to the pending queue.
2205+
pushInterleavedQueue(queue);
2206+
} else {
2207+
update.next = interleaved.next;
2208+
interleaved.next = update;
2209+
}
2210+
queue.interleaved = update;
2211+
} else {
2212+
const pending = queue.pending;
2213+
if (pending === null) {
2214+
// This is the first update. Create a circular list.
2215+
update.next = update;
2216+
} else {
2217+
update.next = pending.next;
2218+
pending.next = update;
2219+
}
2220+
queue.pending = update;
2221+
}
2222+
2223+
if (__DEV__) {
2224+
// $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
2225+
if ('undefined' !== typeof jest) {
2226+
warnIfNotCurrentlyActingUpdatesInDev(fiber);
2227+
}
2228+
}
2229+
const root = scheduleUpdateOnFiber(fiber, lane, eventTime);
2230+
2231+
if (isTransitionLane(lane) && root !== null) {
2232+
let queueLanes = queue.lanes;
2233+
2234+
// If any entangled lanes are no longer pending on the root, then they
2235+
// must have finished. We can remove them from the shared queue, which
2236+
// represents a superset of the actually pending lanes. In some cases we
2237+
// may entangle more than we need to, but that's OK. In fact it's worse if
2238+
// we *don't* entangle when we should.
2239+
queueLanes = intersectLanes(queueLanes, root.pendingLanes);
2240+
2241+
// Entangle the new transition lane with the other transition lanes.
2242+
const newQueueLanes = mergeLanes(queueLanes, lane);
2243+
queue.lanes = newQueueLanes;
2244+
// Even if queue.lanes already include lane, we don't know for certain if
2245+
// the lane finished since the last time we entangled it. So we need to
2246+
// entangle it again, just to be sure.
2247+
markRootEntangled(root, newQueueLanes);
2248+
}
2249+
}
2250+
2251+
if (__DEV__) {
2252+
if (enableDebugTracing) {
2253+
if (fiber.mode & DebugTracingMode) {
2254+
const name = getComponentNameFromFiber(fiber) || 'Unknown';
2255+
logStateUpdateScheduled(name, lane, action);
2256+
}
2257+
}
2258+
}
2259+
2260+
if (enableSchedulingProfiler) {
2261+
markStateUpdateScheduled(fiber, lane);
2262+
}
2263+
}
2264+
2265+
function dispatchSetState<S, A>(
2266+
fiber: Fiber,
2267+
queue: UpdateQueue<S, A>,
2268+
action: A,
2269+
) {
2270+
if (__DEV__) {
2271+
if (typeof arguments[3] === 'function') {
2272+
console.error(
2273+
"State updates from the useState() and useReducer() Hooks don't support the " +
2274+
'second callback argument. To execute a side effect after ' +
2275+
'rendering, declare it in the component body with useEffect().',
2276+
);
2277+
}
2278+
}
2279+
2280+
const eventTime = requestEventTime();
2281+
const lane = requestUpdateLane(fiber);
2282+
2283+
const update: Update<S, A> = {
2284+
lane,
2285+
action,
2286+
hasEagerState: false,
21752287
eagerState: null,
21762288
next: (null: any),
21772289
};
@@ -2241,7 +2353,7 @@ function dispatchAction<S, A>(
22412353
// it, on the update object. If the reducer hasn't changed by the
22422354
// time we enter the render phase, then the eager state can be used
22432355
// without calling the reducer again.
2244-
update.eagerReducer = lastRenderedReducer;
2356+
update.hasEagerState = true;
22452357
update.eagerState = eagerState;
22462358
if (is(eagerState, currentState)) {
22472359
// Fast path. We can bail out without scheduling React to re-render.

‎packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js

Lines changed: 160 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3867,7 +3867,7 @@ describe('ReactHooksWithNoopRenderer', () => {
38673867
});
38683868
});
38693869

3870-
it('eager bailout optimization should always compare to latest rendered reducer', () => {
3870+
it('useReducer does not eagerly bail out of state updates', () => {
38713871
// Edge case based on a bug report
38723872
let setCounter;
38733873
function App() {
@@ -3898,7 +3898,6 @@ describe('ReactHooksWithNoopRenderer', () => {
38983898
'Render: -1',
38993899
'Effect: 1',
39003900
'Reducer: 1',
3901-
'Reducer: 1',
39023901
'Render: 1',
39033902
]);
39043903
expect(ReactNoop).toMatchRenderedOutput('1');
@@ -3911,12 +3910,170 @@ describe('ReactHooksWithNoopRenderer', () => {
39113910
'Render: 1',
39123911
'Effect: 2',
39133912
'Reducer: 2',
3914-
'Reducer: 2',
39153913
'Render: 2',
39163914
]);
39173915
expect(ReactNoop).toMatchRenderedOutput('2');
39183916
});
39193917

3918+
it('useReducer does not replay previous no-op actions when other state changes', () => {
3919+
let increment;
3920+
let setDisabled;
3921+
3922+
function Counter() {
3923+
const [disabled, _setDisabled] = useState(true);
3924+
const [count, dispatch] = useReducer((state, action) => {
3925+
if (disabled) {
3926+
return state;
3927+
}
3928+
if (action.type === 'increment') {
3929+
return state + 1;
3930+
}
3931+
return state;
3932+
}, 0);
3933+
3934+
increment = () => dispatch({type: 'increment'});
3935+
setDisabled = _setDisabled;
3936+
3937+
Scheduler.unstable_yieldValue('Render disabled: ' + disabled);
3938+
Scheduler.unstable_yieldValue('Render count: ' + count);
3939+
return count;
3940+
}
3941+
3942+
ReactNoop.render(<Counter />);
3943+
expect(Scheduler).toFlushAndYield([
3944+
'Render disabled: true',
3945+
'Render count: 0',
3946+
]);
3947+
expect(ReactNoop).toMatchRenderedOutput('0');
3948+
3949+
act(() => {
3950+
// These increments should have no effect, since disabled=true
3951+
increment();
3952+
increment();
3953+
increment();
3954+
});
3955+
expect(Scheduler).toHaveYielded([
3956+
'Render disabled: true',
3957+
'Render count: 0',
3958+
]);
3959+
expect(ReactNoop).toMatchRenderedOutput('0');
3960+
3961+
act(() => {
3962+
// Enabling the updater should *not* replay the previous increment() actions
3963+
setDisabled(false);
3964+
});
3965+
expect(Scheduler).toHaveYielded([
3966+
'Render disabled: false',
3967+
'Render count: 0',
3968+
]);
3969+
expect(ReactNoop).toMatchRenderedOutput('0');
3970+
});
3971+
3972+
it('useReducer does not replay previous no-op actions when props change', () => {
3973+
let setDisabled;
3974+
let increment;
3975+
3976+
function Counter({disabled}) {
3977+
const [count, dispatch] = useReducer((state, action) => {
3978+
if (disabled) {
3979+
return state;
3980+
}
3981+
if (action.type === 'increment') {
3982+
return state + 1;
3983+
}
3984+
return state;
3985+
}, 0);
3986+
3987+
increment = () => dispatch({type: 'increment'});
3988+
3989+
Scheduler.unstable_yieldValue('Render count: ' + count);
3990+
return count;
3991+
}
3992+
3993+
function App() {
3994+
const [disabled, _setDisabled] = useState(true);
3995+
setDisabled = _setDisabled;
3996+
Scheduler.unstable_yieldValue('Render disabled: ' + disabled);
3997+
return <Counter disabled={disabled} />;
3998+
}
3999+
4000+
ReactNoop.render(<App />);
4001+
expect(Scheduler).toFlushAndYield([
4002+
'Render disabled: true',
4003+
'Render count: 0',
4004+
]);
4005+
expect(ReactNoop).toMatchRenderedOutput('0');
4006+
4007+
act(() => {
4008+
// These increments should have no effect, since disabled=true
4009+
increment();
4010+
increment();
4011+
increment();
4012+
});
4013+
expect(Scheduler).toHaveYielded(['Render count: 0']);
4014+
expect(ReactNoop).toMatchRenderedOutput('0');
4015+
4016+
act(() => {
4017+
// Enabling the updater should *not* replay the previous increment() actions
4018+
setDisabled(false);
4019+
});
4020+
expect(Scheduler).toHaveYielded([
4021+
'Render disabled: false',
4022+
'Render count: 0',
4023+
]);
4024+
expect(ReactNoop).toMatchRenderedOutput('0');
4025+
});
4026+
4027+
it('useReducer applies potential no-op changes if made relevant by other updates in the batch', () => {
4028+
let setDisabled;
4029+
let increment;
4030+
4031+
function Counter({disabled}) {
4032+
const [count, dispatch] = useReducer((state, action) => {
4033+
if (disabled) {
4034+
return state;
4035+
}
4036+
if (action.type === 'increment') {
4037+
return state + 1;
4038+
}
4039+
return state;
4040+
}, 0);
4041+
4042+
increment = () => dispatch({type: 'increment'});
4043+
4044+
Scheduler.unstable_yieldValue('Render count: ' + count);
4045+
return count;
4046+
}
4047+
4048+
function App() {
4049+
const [disabled, _setDisabled] = useState(true);
4050+
setDisabled = _setDisabled;
4051+
Scheduler.unstable_yieldValue('Render disabled: ' + disabled);
4052+
return <Counter disabled={disabled} />;
4053+
}
4054+
4055+
ReactNoop.render(<App />);
4056+
expect(Scheduler).toFlushAndYield([
4057+
'Render disabled: true',
4058+
'Render count: 0',
4059+
]);
4060+
expect(ReactNoop).toMatchRenderedOutput('0');
4061+
4062+
act(() => {
4063+
// Although the increment happens first (and would seem to do nothing since disabled=true),
4064+
// because these calls are in a batch the parent updates first. This should cause the child
4065+
// to re-render with disabled=false and *then* process the increment action, which now
4066+
// increments the count and causes the component output to change.
4067+
increment();
4068+
setDisabled(false);
4069+
});
4070+
expect(Scheduler).toHaveYielded([
4071+
'Render disabled: false',
4072+
'Render count: 1',
4073+
]);
4074+
expect(ReactNoop).toMatchRenderedOutput('1');
4075+
});
4076+
39204077
// Regression test. Covers a case where an internal state variable
39214078
// (`didReceiveUpdate`) is not reset properly.
39224079
it('state bail out edge case (#16359)', async () => {

0 commit comments

Comments
 (0)
Please sign in to comment.