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

Performance issue (crash) in large apps when using Field with children in React v16 #3461

Closed
flut1 opened this Issue Sep 28, 2017 · 18 comments

Comments

9 participants
@flut1

flut1 commented Sep 28, 2017

Are you submitting a bug report or a feature request?

bug

What is the current behavior?

Reproduction:

  • Use redux-form with React v16 in a large application (a page with a lot of different nested React components)
  • Add a field with children:
<Field component={SomeCustomInput} ...>
   <SomeChild \>
   <SomeChild \>
   <SomeChild \>
</Field>
  • The page becomes so slow that the browser becomes unresponsive and crashes

What seems to happen is that <ConnectedField /> will execute shouldComponentUpdate, which will call lodash isEqual on its props. The children prop in react v16 contains FiberNode objects, which seem to have references to a lot (if not all) other FiberNode on the page. Because of this, lodash isEqual will take a very very long time recursively traversing through these objects.

Sandbox Link

Unfortunately I haven't been able to reproduce this in stand-alone example. If I create a <Field> with children in one of the examples of this repo, I can see lodash baseIsEqualDeep being called with very long call stacks. However, it is not enough to visually slow down the page.

What's your environment?

All browsers
React v16
redux-form v7.0.4

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 5, 2017

Contributor

Please don't try to do deep equality checks on React data structures. It was never intentionally supported. The only way you can get to FiberNodes is via _owner property on an element. It is considered internal, and you should never traverse it. It happened to work in 15 but even in 15 it was very inefficient because you compared internal instances. In 16, there are more potential paths between nodes so this is how you encounter it.

Contributor

gaearon commented Oct 5, 2017

Please don't try to do deep equality checks on React data structures. It was never intentionally supported. The only way you can get to FiberNodes is via _owner property on an element. It is considered internal, and you should never traverse it. It happened to work in 15 but even in 15 it was very inefficient because you compared internal instances. In 16, there are more potential paths between nodes so this is how you encounter it.

@erikras

This comment has been minimized.

Show comment
Hide comment
@erikras

erikras Oct 5, 2017

Owner

... when the calculation to decide not to render takes up more time than actually doing the render ... 🤦‍♂️

Owner

erikras commented Oct 5, 2017

... when the calculation to decide not to render takes up more time than actually doing the render ... 🤦‍♂️

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 5, 2017

Contributor

Yeah, this is why we don't recommend deep equality checks 😛

Contributor

gaearon commented Oct 5, 2017

Yeah, this is why we don't recommend deep equality checks 😛

@oieduardorabelo

This comment has been minimized.

Show comment
Hide comment
@oieduardorabelo

oieduardorabelo Oct 5, 2017

doing consulting, often I see:

shouldComponentUpdate(nextProps) {
  if (JSON.stringify(nextProps) !== JSON.stringify(this.props) {
    ....
  }
}

can be as bad as this one...

this issue will be another good argument for this scenario 😅

oieduardorabelo commented Oct 5, 2017

doing consulting, often I see:

shouldComponentUpdate(nextProps) {
  if (JSON.stringify(nextProps) !== JSON.stringify(this.props) {
    ....
  }
}

can be as bad as this one...

this issue will be another good argument for this scenario 😅

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 5, 2017

Contributor

Oh god.

Contributor

gaearon commented Oct 5, 2017

Oh god.

@umidbekkarimov

This comment has been minimized.

Show comment
Hide comment
@umidbekkarimov

umidbekkarimov Oct 5, 2017

Contributor

Or just check react elements in deepEqual

// @flow
+ import React from 'react'
import { isEqualWith } from 'lodash'

const customizer = (obj: any, other: any) => {
  if (obj === other) return true
  if (
    (obj == null || obj === '' || obj === false) &&
    (other == null || other === '' || other === false)
  ) {
    return !(
      (obj === undefined && other === false) ||
      (obj === false && other === undefined)
    )
  }

  if (obj && other && obj._error !== other._error) return false
  if (obj && other && obj._warning !== other._warning) return false
+  if (React.isValidElement(obj) || React.isValidElement(other)) return false
}

const deepEqual = (a: any, b: any) => isEqualWith(a, b, customizer)

export default deepEqual
Contributor

umidbekkarimov commented Oct 5, 2017

Or just check react elements in deepEqual

// @flow
+ import React from 'react'
import { isEqualWith } from 'lodash'

const customizer = (obj: any, other: any) => {
  if (obj === other) return true
  if (
    (obj == null || obj === '' || obj === false) &&
    (other == null || other === '' || other === false)
  ) {
    return !(
      (obj === undefined && other === false) ||
      (obj === false && other === undefined)
    )
  }

  if (obj && other && obj._error !== other._error) return false
  if (obj && other && obj._warning !== other._warning) return false
+  if (React.isValidElement(obj) || React.isValidElement(other)) return false
}

const deepEqual = (a: any, b: any) => isEqualWith(a, b, customizer)

export default deepEqual
@erikras

This comment has been minimized.

Show comment
Hide comment
@erikras

erikras Oct 5, 2017

Owner

@umidbekkarimov Interesting approach, but I'm pretty happy with my solution in #3480. If there are (or were) children, shouldComponentUpdate() must return true.

Owner

erikras commented Oct 5, 2017

@umidbekkarimov Interesting approach, but I'm pretty happy with my solution in #3480. If there are (or were) children, shouldComponentUpdate() must return true.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 5, 2017

Contributor

What if the prop is not called children? Custom component could receive another prop with React elements, e.g. <Input placeholder={<span />} />.

Contributor

gaearon commented Oct 5, 2017

What if the prop is not called children? Custom component could receive another prop with React elements, e.g. <Input placeholder={<span />} />.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 5, 2017

Contributor

Why can't this be a simple shallow equality check?

Contributor

gaearon commented Oct 5, 2017

Why can't this be a simple shallow equality check?

@iamtommcc

This comment has been minimized.

Show comment
Hide comment
@iamtommcc

iamtommcc Oct 5, 2017

@gaearon In the example you provided (<Input placeholder={<span />} />), wouldn't a shallow equality check never work since createElement is being called every render for that span? Or have I totally misunderstood?

iamtommcc commented Oct 5, 2017

@gaearon In the example you provided (<Input placeholder={<span />} />), wouldn't a shallow equality check never work since createElement is being called every render for that span? Or have I totally misunderstood?

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Oct 5, 2017

I think @gaearon meant that no deep equality check should happen, regardless the prop name. If you want to preserve identities - just cache ur complex data structures (like <span/>) in ur example, so they can be shallowly equaled between renders.

Andarist commented Oct 5, 2017

I think @gaearon meant that no deep equality check should happen, regardless the prop name. If you want to preserve identities - just cache ur complex data structures (like <span/>) in ur example, so they can be shallowly equaled between renders.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 5, 2017

Contributor

Yes. I'm saying that, unless it is your component and you're in control of its API, you shouldn't assume that the only place where React elements can appear is this.props.children. They could be in any prop (or in some nested data structure inside a prop).

To me, it seems like it is never safe to do deep equality checks on props. And we never recommended doing that either.

Contributor

gaearon commented Oct 5, 2017

Yes. I'm saying that, unless it is your component and you're in control of its API, you shouldn't assume that the only place where React elements can appear is this.props.children. They could be in any prop (or in some nested data structure inside a prop).

To me, it seems like it is never safe to do deep equality checks on props. And we never recommended doing that either.

@erikras

This comment has been minimized.

Show comment
Hide comment
@erikras

erikras Oct 5, 2017

Owner

Good arguments, @gaearon. Implemented @umidbekkarimov's idea in #3481.

Owner

erikras commented Oct 5, 2017

Good arguments, @gaearon. Implemented @umidbekkarimov's idea in #3481.

@erikras

This comment has been minimized.

Show comment
Hide comment
@erikras

erikras Oct 6, 2017

Owner

Published fix in v7.1.0.

Owner

erikras commented Oct 6, 2017

Published fix in v7.1.0.

@kamleshchandnani

This comment has been minimized.

Show comment
Hide comment
@kamleshchandnani

kamleshchandnani Oct 8, 2017

If deep comparision or JSON.stringify() is not a good approach then how can we do an equality check? It's not necessary that everytime by shallow compare we can decide whether to update the component from shouldComponentUpdate()

kamleshchandnani commented Oct 8, 2017

If deep comparision or JSON.stringify() is not a good approach then how can we do an equality check? It's not necessary that everytime by shallow compare we can decide whether to update the component from shouldComponentUpdate()

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Oct 8, 2017

Why shallow comparison would not be good enough? If you prefer immutability (and I think you should, it's also rather preferred way in React's ecosystem), you can easily rely on shallow comparisons.

Andarist commented Oct 8, 2017

Why shallow comparison would not be good enough? If you prefer immutability (and I think you should, it's also rather preferred way in React's ecosystem), you can easily rely on shallow comparisons.

@kamleshchandnani

This comment has been minimized.

Show comment
Hide comment
@kamleshchandnani

kamleshchandnani Oct 8, 2017

@Andarist Assuming if my object on which the react component depends is of the form themeObj:{theme:{"header":{"leftHeaderColor":"red","rightHeaderColor":"black"}}} and I have to return true only when rightHeaderColor is changed, but this will fail in shallow compare

kamleshchandnani commented Oct 8, 2017

@Andarist Assuming if my object on which the react component depends is of the form themeObj:{theme:{"header":{"leftHeaderColor":"red","rightHeaderColor":"black"}}} and I have to return true only when rightHeaderColor is changed, but this will fail in shallow compare

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot Oct 8, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

lock bot commented Oct 8, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 8, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.