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.

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

Don't diff memoized host components in completion phase #13423

Merged
merged 6 commits into from Aug 17, 2018

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 17, 2018

Fixes #13425.

View without whitespace: https://github.com/facebook/react/pull/13423/files?w=1

The bigger issue (#13424) is still a problem but it has always been broken (I checked as far as 0.11). Here I'm only fixing the regression in React 16 that made this a problem even when setState calls are in different components.

In particular, we shouldn't be diffing host components that are outside of setState path.

Some indentation changes are due to tests I tweaked but they're unrelated. The only change there is to consistently add and remove container from the document. The real changes are only in the newly added test at the bottom, and in CompleteWork.

@gaearon gaearon changed the title Don't diff memoized host components Don't diff memoized host components in completion phase Aug 17, 2018
@@ -357,8 +357,8 @@ exports[`ReactDebugFiberPerf skips parents during setState 1`] = `

⚛ (Committing Changes)
⚛ (Committing Snapshot Effects: 0 Total)
⚛ (Committing Host Effects: 6 Total)
⚛ (Calling Lifecycle Methods: 6 Total)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should have spotted this earlier. Sigh.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just way more efficient now?

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 not — I think it was still overcounting actual updates (not entirely sure why). But it's better.

@pull-bot
Copy link

pull-bot commented Aug 17, 2018

Details of bundled changes.

Comparing: d14e443...0a477d8

react-noop-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-noop-renderer.development.js +2.1% +1.5% 22.88 KB 23.36 KB 5.25 KB 5.33 KB NODE_DEV
react-noop-renderer.production.min.js 🔺+1.6% 🔺+1.9% 8.62 KB 8.75 KB 2.98 KB 3.03 KB NODE_PROD
react-noop-renderer-persistent.development.js +2.1% +1.5% 23.01 KB 23.48 KB 5.26 KB 5.34 KB NODE_DEV
react-noop-renderer-persistent.production.min.js 🔺+1.5% 🔺+1.8% 8.64 KB 8.77 KB 2.98 KB 3.04 KB NODE_PROD

Generated by 🚫 dangerJS

@@ -361,6 +361,10 @@ function completeWork(
// If we have an alternate, that means this is an update and we need to
// schedule a side-effect to do the updates.
const oldProps = current.memoizedProps;
if (oldProps === newProps) {
break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Be careful with those early returns. I think you’re skipping the ref changing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can it even change if we bail out?

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'll push up a fix tho.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In theory we can have a different element with the same props object.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Seems legit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Host components outside the setState path are sometimes unnecessarily diffed and updated
5 participants