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
Member

@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
Member Author

@gaearon gaearon Aug 17, 2018

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.

Loading

Copy link
Contributor

@nhunzaker nhunzaker Aug 17, 2018

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?

Loading

Copy link
Member Author

@gaearon gaearon Aug 17, 2018

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.

Loading

@pull-bot
Copy link

@pull-bot 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

Loading

@@ -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
Member

@sebmarkbage sebmarkbage Aug 17, 2018

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.

Loading

Copy link
Member Author

@gaearon gaearon Aug 17, 2018

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?

Loading

Copy link
Member Author

@gaearon gaearon Aug 17, 2018

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.

Loading

Copy link
Member

@sebmarkbage sebmarkbage Aug 17, 2018

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.

Loading

Copy link
Member

@sebmarkbage sebmarkbage left a comment

Seems legit

Loading

@gaearon gaearon merged commit d2f5c3f into facebook:master Aug 17, 2018
1 check passed
Loading
@gaearon gaearon deleted the fix-host-skip branch Aug 17, 2018
TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 2018
* Add a regression test for 12643#issuecomment-413727104

* Don't diff memoized host components

* Add regression tests for noop renderer

* No early return

* Strengthen the test for host siblings

* Flow types
@gaearon gaearon mentioned this pull request Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants