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

fix: don't run effects if a render phase update results in unchanged deps #20676

Merged
merged 9 commits into from
Jan 29, 2021
Merged

fix: don't run effects if a render phase update results in unchanged deps #20676

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c8435e7
current behavior of effect dependencies on render phase updates
eps1lon Jan 27, 2021
5bcce83
fix: don't schedule effects when render phase updates aren't committed
eps1lon Jan 27, 2021
6cfd74d
apply fix to new fork
eps1lon Jan 27, 2021
9dfcd56
Use suggested test name
eps1lon Jan 27, 2021
0f58f40
devtools: compare effects by deps not referential equality
eps1lon Jan 27, 2021
3cdd353
disable type checking for added functions
eps1lon Jan 27, 2021
e39a670
bump CircleCI
eps1lon Jan 27, 2021
77ed727
Be more defensive about `useState({ deps })`
eps1lon Jan 28, 2021
5c24bb5
bail out early if memoizedState didn't change
eps1lon Jan 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 45 additions & 1 deletion packages/react-devtools-shared/src/backend/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ import type {
ComponentFilter,
ElementType,
} from 'react-devtools-shared/src/types';
import is from 'shared/objectIs';

type getDisplayNameForFiberType = (fiber: Fiber) => string | null;
type getTypeSymbolType = (type: any) => Symbol | number;
Expand Down Expand Up @@ -1073,6 +1074,49 @@ export function attach(
return null;
}

function areHookInputsEqual(
nextDeps: Array<mixed>,
prevDeps: Array<mixed> | null,
) {
if (prevDeps === null) {
return false;
}

for (let i = 0; i < prevDeps.length && i < nextDeps.length; i++) {
if (is(nextDeps[i], prevDeps[i])) {
continue;
}
return false;
}
return true;
}

function isEffect(memoizedState) {
return (
memoizedState !== null &&
typeof memoizedState === 'object' &&
memoizedState.hasOwnProperty('tag') &&
memoizedState.hasOwnProperty('create') &&
memoizedState.hasOwnProperty('destroy') &&
memoizedState.hasOwnProperty('deps') &&
(memoizedState.deps === null || Array.isArray(memoizedState.deps)) &&
memoizedState.hasOwnProperty('next')
);
}
bvaughn marked this conversation as resolved.
Show resolved Hide resolved

function didHookChange(prev: any, next: any): boolean {
const prevMemoizedState = prev.memoizedState;
const nextMemoizedState = next.memoizedState;

if (isEffect(prevMemoizedState) && isEffect(nextMemoizedState)) {
Copy link
Contributor

@bvaughn bvaughn Jan 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

When would the return values for isEffect(prevMemoizedState) and isEffect(nextMemoizedState) ever differ? Can we just check isEffect(nextMemoizedState) here?

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 so. But I haven't checked if Effect hooks start with memoizedState = null. I'll see if can improve the type story here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If they did, that wouldn't matter here because we early return in didHooksChange if either one was null.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just do this tweak in a follow up? :) Would be nice.

return !areHookInputsEqual(
nextMemoizedState.deps,
prevMemoizedState.deps,
);
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not a fan of this new coupling. Especially since this will not work as expected for e.g. useState({ tag, create, destroy, deps, next }); even though that's unlikely to occur in userland. @bvaughn Any suggestions how this should work now that hook.memoizedState always changes even if deps didn't?

Copy link
Collaborator

@acdlite acdlite Jan 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

DevTools probably should be resilient to this happening, however, if you want to kick the can down the road, another solution could be to assign hook.memoizedState = currentHook.memoizedState when you bailout. Instead of the result of pushEffect. In the case of of a normal update, it's a no-op because that's what it already points to. But for render phase updates, it will restore the original object.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you end up doing this, please leave a TODO here so we remember this is accidentally coupled

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But for render phase updates, it will restore the original object.

This was actually my first intuition but I didn't really grasp how hook relates to currentHook.

But since you seem to be ok with the current approach I'm keeping the initial fix. I added an additional type check to memoizedState.deps in case someone has useState({ deps: 1, tag, create, destroy, next });

Copy link
Contributor

Choose a reason for hiding this comment

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

Still planning on looking at this question. It's next in my queue.

Copy link
Contributor

Choose a reason for hiding this comment

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

At first I thought this might break the Profiler too (how it determines what rendered and what didn't) but I forgot that uses the special PerformedWork tag.

return nextMemoizedState !== prevMemoizedState;
}

function didHooksChange(prev: any, next: any): boolean {
if (prev == null || next == null) {
return false;
Expand All @@ -1086,7 +1130,7 @@ export function attach(
next.hasOwnProperty('queue')
) {
while (next !== null) {
if (next.memoizedState !== prev.memoizedState) {
if (didHookChange(prev, next)) {
return true;
} else {
next = next.next;
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberHooks.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1328,7 +1328,7 @@ function updateEffectImpl(fiberFlags, hookFlags, create, deps): void {
if (nextDeps !== null) {
const prevDeps = prevEffect.deps;
if (areHookInputsEqual(nextDeps, prevDeps)) {
pushEffect(hookFlags, create, destroy, nextDeps);
hook.memoizedState = pushEffect(hookFlags, create, destroy, nextDeps);
Copy link
Collaborator Author

@eps1lon eps1lon Jan 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This doesn't seem like the correct approach. I would rather restore the memoized state when we trigger a render phase update.

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 tried

-        pushEffect(hookFlags, create, destroy, nextDeps);
+        const effect = pushEffect(hookFlags, create, destroy, nextDeps);
+        if (!areHookInputsEqual(effect.deps, hook.memoizedState.deps)) {
+          hook.memoizedState = effect;
+        }

locally which makes more sense conceptually. However, this results in a duplicate warning in

react/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js

Lines 614 to 622 in d17086c

expect(() => {
root.update(<App dependencies={['A', 'B']} />);
}).toErrorDev([
'Warning: The final argument passed to useLayoutEffect changed size ' +
'between renders. The order and size of this array must remain ' +
'constant.\n\n' +
'Previous: [A]\n' +
'Incoming: [A, B]\n',
]); 

-        pushEffect(hookFlags, create, destroy, nextDeps);
+        const effect = pushEffect(hookFlags, create, destroy, nextDeps);
+        if (
+          didScheduleRenderPhaseUpdate &&
+          !areHookInputsEqual(effect.deps, hook.memoizedState.deps)
+        ) {
+          hook.memoizedState = effect;
+        }

passes all tests locally but seems even more sketchy.

Copy link
Collaborator

@acdlite acdlite Jan 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The fix you committed is more correct. We should compare against the current hook, not the hook from the pass where the render phase update happened.

The bug is that in the case of a render phase update, we're not persisting the most current hook. We're persisting the "intermediate" hook that happens before the render phase update replaces it.

Is there a reason you don't like your fix that I'm overlooking?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a reason you don't like your fix that I'm overlooking?

The failed devtools test prompted my response. Right now we're invalidating the memoizedState even though the deps didn't change. I suspected that this may cause performance regressions in other places considering that everywhere we compare hook.memoizedState === prevHook.memoizedState it'll now evaluate to false even though the deps didn't change.

But maybe memoizedState was never intended to be used that way. It might make sense to compare the actual deps not just memoizedState in devtools.

I'll look into the test now. Thanks for the clarification 👍

return;
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberHooks.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1305,7 +1305,7 @@ function updateEffectImpl(fiberFlags, hookFlags, create, deps): void {
if (nextDeps !== null) {
const prevDeps = prevEffect.deps;
if (areHookInputsEqual(nextDeps, prevDeps)) {
pushEffect(hookFlags, create, destroy, nextDeps);
hook.memoizedState = pushEffect(hookFlags, create, destroy, nextDeps);
return;
}
}
Expand Down
43 changes: 43 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3760,4 +3760,47 @@ describe('ReactHooksWithNoopRenderer', () => {
// effects would not fire.
expect(Scheduler).toHaveYielded(['Unmount A', 'Unmount B']);
});

it('effect dependencies are persisted after a render phase update', () => {
let handleClick;
function Test() {
const [count, setCount] = useState(0);

useEffect(() => {
Scheduler.unstable_yieldValue(`Effect: ${count}`);
}, [count]);

if (count > 0) {
setCount(0);
}

handleClick = () => setCount(2);

return <Text text={`Render: ${count}`} />;
}

ReactNoop.act(() => {
ReactNoop.render(<Test />);
});

expect(Scheduler).toHaveYielded(['Render: 0', 'Effect: 0']);

ReactNoop.act(() => {
handleClick();
});

expect(Scheduler).toHaveYielded(['Render: 0']);

ReactNoop.act(() => {
handleClick();
});

expect(Scheduler).toHaveYielded(['Render: 0']);

ReactNoop.act(() => {
handleClick();
});

expect(Scheduler).toHaveYielded(['Render: 0']);
});
});