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

[EnhancedButton] Speed up unmount #6164

Merged
merged 2 commits into from
Feb 16, 2017
Merged

[EnhancedButton] Speed up unmount #6164

merged 2 commits into from
Feb 16, 2017

Conversation

fzaninotto
Copy link
Contributor

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

While profiling a moderate size page built with material-ui, I noticed that a significant amount of time is spent unmounting mui components. The culprit is EnhancedButton, which calls clearTimeout() in componentWillUnmount(). This represents 30% of the unmount time, about 25ms on a page with less than a dozen buttons:

unmount_initial

It turns out that most of the time, the timeout that EnhancedButton wants to clean doesn't exist (it's the one created for the hover ripple effect). Adding an if before the call to clearTimeout reduces the time spent in EnhancedButton to something insignificant, and reduces the overall unmount time by 50%:

unmount_improved

This may look like a small change, but as EnhancedButton powers many other components, it has important effects.

@muibot muibot added PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Feb 16, 2017
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Nice finding 👍 .

@oliviertassinari oliviertassinari added PR: Review Accepted and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Feb 16, 2017
@oliviertassinari oliviertassinari merged commit 7048e9e into mui:master Feb 16, 2017
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.

None yet

3 participants