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

[LeftNav] move unnecessary state to instance properties. #1184

Merged
merged 1 commit into from
Jul 17, 2015
Merged

[LeftNav] move unnecessary state to instance properties. #1184

merged 1 commit into from
Jul 17, 2015

Conversation

maoziliang
Copy link
Contributor

For performance.
I think the properties that not trigger component's render should not belong to component's state.
This can reduce many render cycles.

@hai-cea
Copy link
Member

hai-cea commented Jul 16, 2015

@pomerantsev What do you think about this change?

@hai-cea hai-cea changed the title move left-nav unnecessary state to instance properties. [LeftNav] move unnecessary state to instance properties. Jul 16, 2015
@pomerantsev
Copy link
Contributor

Actually, I was just used to storing any local state properties on the state object, that's why I did it that way.
@maoziliang: correct me if I'm wrong, but I think one of the points of introducing the state object in React was to have a single place to store a component's state.
Also, you claim there's a performance gain. I don't see any difference in performance in both cases where you've eliminated setState (tested your branch on two devices, and there's no visual difference, but I didn't do any measurements). Is there anything to prove your approach is better? Some articles maybe? Or maybe you ran some performance tests?
I'm curious to know if people do it differently.

@cgestes
Copy link
Contributor

cgestes commented Jul 16, 2015

@pomerantsev use React.tools.Perfs to measure performance.

From what I understand about reading the code, the changeset make a lot of sense.

State should only include props that trigger a rerender or a lot of rerender are done for nothing.

(and there are many many useless rerender in mui at the moment, it would be nice to lower them)

Optimally when the render method is called it must produced a different output than the previous call.

@cgestes
Copy link
Contributor

cgestes commented Jul 16, 2015

to say it in the reverse way, state should only includes properties used in the render function.

@pomerantsev
Copy link
Contributor

@cgestes: cool, I'll take a look at the Perf stuff. I totally understand what you're saying about the render method.
Anyway, this PR seems to be safe, it doesn't change the logic.
If you guys feel that it will make the code cleaner, I'm all for it.

@oliviertassinari
Copy link
Member

I agree with @cgestes the state should only contains properties used in the render method.
👍

@maoziliang
Copy link
Contributor Author

Agree with @cgestes.
I'll upload chrome dev Timeline screenshots here later. It improves so much performance and has obvious different user experience in not well implemented(pool performance) webview and browser.React render cycle is the bottleneck of performance.

@maoziliang
Copy link
Contributor Author

  • All properties in state. The timeline screenshot.
    All on state screenshot
  • Some properties move out. touchstart and touchmove timeline screenshots.
    touchstart screenshot
    touchmove sceenshot

The difference of JS Frame between these two version tell us the truth.

@pomerantsev
Copy link
Contributor

Great. @maoziliang, thanks for the optimization!

@hai-cea
Copy link
Member

hai-cea commented Jul 17, 2015

Thanks @maoziliang @cgestes @pomerantsev @oliviertassinari - you guys are awesome! 👍

hai-cea added a commit that referenced this pull request Jul 17, 2015
[LeftNav] move unnecessary state to instance properties.
@hai-cea hai-cea merged commit 2585d91 into mui:master Jul 17, 2015
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Nov 10, 2020
* Correct position of inline wrapper popover

* Adjust variant prop description

* Adjust props description

* Uncomment example

* Add prop-types.json to prettier-ignore

* Properly freeze clock for percy
@zannager zannager added the docs Improvements or additions to the documentation label Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants