Skip to content

Commit

Permalink
Avoid deopt on changed getSnapshot function unless snapshot also changes
Browse files Browse the repository at this point in the history
getSnapshot should be memoized to only change when its inputs change, but if that memoization is done incorrectly (or if the new snapshot just happens to be the same regardless of a changed dependency) then we can avoid deopting to a root re-render.
  • Loading branch information
Brian Vaughn committed Feb 21, 2020
1 parent 008cc36 commit 6283202
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 6 deletions.
20 changes: 14 additions & 6 deletions packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -1060,17 +1060,25 @@ function useMutableSourceImpl<Source, Snapshot>(
subscribe,
});
} else if (state.getSnapshot !== getSnapshot) {
// TODO (useMutableSource) We could avoid throwing if the snapshot doesn't change.
const snapshot = getSnapshot(source._source);
setState({
getSnapshot,
snapshot: getSnapshot(source._source),
snapshot,
source,
subscribe,
});
invariant(
false,
'Cannot read from mutable source during the current render without tearing. This is a bug in React. Please file an issue.',
);

// Normally a new getSnapshot function implies a new snapshot value.
// This function should be memoized so that it only changes when its dependencies change.
// Tf the returned snapshot value is the same-
// it likely means getSnapshot is not memoized correctly (e.g. inline function).
// In this case we can avoid the expensive deopt and just update local component state.
if (state.snapshot !== snapshot) {
invariant(
false,
'Cannot read from mutable source during the current render without tearing. This is a bug in React. Please file an issue.',
);
}
}

return state.snapshot;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ function loadModules({
ReactFeatureFlags.flushSuspenseFallbacksInTests = false;
ReactFeatureFlags.deferPassiveEffectCleanupDuringUnmount = deferPassiveEffectCleanupDuringUnmount;
ReactFeatureFlags.runAllPassiveEffectDestroysBeforeCreates = runAllPassiveEffectDestroysBeforeCreates;
ReactFeatureFlags.enableProfilerTimer = true;
React = require('react');
ReactNoop = require('react-noop-renderer');
Scheduler = require('scheduler');
Expand Down Expand Up @@ -3093,6 +3094,64 @@ function loadModules({
});
});

it('should not throw if the new getSnapshot returns the same snapshot value', () => {
const source = createSource('one');
const mutableSource = createMutableSource(source);

const onRenderA = jest.fn();
const onRenderB = jest.fn();

let updateGetSnapshot;

function WrapperWithState() {
const tuple = React.useState(() => defaultGetSnapshot);
updateGetSnapshot = tuple[1];
return (
<Component
label="b"
getSnapshot={tuple[0]}
mutableSource={mutableSource}
subscribe={defaultSubscribe}
/>
);
}

act(() => {
ReactNoop.render(
<>
<React.Profiler id="a" onRender={onRenderA}>
<Component
label="a"
getSnapshot={defaultGetSnapshot}
mutableSource={mutableSource}
subscribe={defaultSubscribe}
/>
</React.Profiler>
<React.Profiler id="b" onRender={onRenderB}>
<WrapperWithState />
</React.Profiler>
</>,
() => Scheduler.unstable_yieldValue('Sync effect'),
);
expect(Scheduler).toFlushAndYield([
'a:one',
'b:one',
'Sync effect',
]);
ReactNoop.flushPassiveEffects();
expect(onRenderA).toHaveBeenCalledTimes(1);
expect(onRenderB).toHaveBeenCalledTimes(1);

// If B's getSnapshot function updates, but the snapshot it returns is the same,
// only B should re-render (to update its state).
updateGetSnapshot(() => s => defaultGetSnapshot(s));
expect(Scheduler).toFlushAndYield(['b:one', 'b:one']);
ReactNoop.flushPassiveEffects();
expect(onRenderA).toHaveBeenCalledTimes(1);
expect(onRenderB).toHaveBeenCalledTimes(2);
});
});

it('should still schedule an update if an eager selector throws after a mutation', () => {
const source = createSource({
friends: [
Expand Down

0 comments on commit 6283202

Please sign in to comment.