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

Tab componentDidUpdate forces reflow #11673

Closed
toddtarsi opened this issue Jun 1, 2018 · 10 comments
Closed

Tab componentDidUpdate forces reflow #11673

toddtarsi opened this issue Jun 1, 2018 · 10 comments
Labels
component: tabs This is the name of the generic UI component, not the React module! performance

Comments

@toddtarsi
Copy link

Hi, great repo and nice rollout of 1.x! I'm liking this repo a lot, but I have a slight issue with the Tabs implementation, in that it forces a DOM thrash on the componentDidUpdate call.

https://github.com/mui-org/material-ui/blob/303199d39b42a321d28347d8440d69166f872f27/packages/material-ui/src/Tab/Tab.js#L100

This getBoundingClientRects call is really hitting me pretty hard on Android. Is there anyway I could modify this component, and add a property like 'ignoreChanges', to just not call this on componentDidUpdate at all? Performance is that important to me at this point. I could also PR this, if that would help. I enjoy the chance to help out.

@mbrookes mbrookes added performance component: tabs This is the name of the generic UI component, not the React module! labels Jun 1, 2018
@oliviertassinari
Copy link
Member

@toddtarsi I'm not against the option of adding a disableLabelWrapping?: boolean property.

This getBoundingClientRects call is really hitting me pretty hard on Android.

Do you have some benchmark/monitoring graph to confirm this?

@toddtarsi
Copy link
Author

toddtarsi commented Jun 1, 2018

I have been running some profiling, but its on Android against a heavy UX, so it's not really a good reproduction. If you'd like, I can produce a more directed benchmark this weekend to demonstrate.
However, for transparency, I'll share how I've benchmarked thus far.

We have a moderately complex application, and on this page, there's about 10 list items (love the new list items btw!), a drawer, 3 tabs, maybe 10 buttons, and a few drop down menus. So, the render times can get pretty rough on low end Android.

Basically, I have just been profiling via Chrome debugger tools, chasing down the call trees, and looking for any unneeded DOM thrashing. The purple is DOM thrashing, which came down to two sources. Button ripples and Tab. Button ripples were easy to deal with.

Android running just like web:

screen shot 2018-06-01 at 5 53 27 pm

After disabling touch ripple on list items, menu items, and buttons, but still with tabs.
I unfortunately didn't save this exact profile, or else I could give a better estimate of the DOM savings I got, but it cut out about a full second, very nice!

screen shot 2018-06-01 at 3 30 42 pm

After disabling the touch ripple on tabs:

image

You can see there's still some purple, because I missed another DOM thrash that happens in getTabsMeta (unfortunately, I just tested the componentDidUpdate to see if there were significant savings). That said, I got about a half second off there.

Also I should mention, all the yellow is just me doing too many cache writes and causing a couple of vdom renders before it settles down. However, on the tabs, the name text never changes, and since its on Android, it never resizes either, so I know for a fact that its safe on spacing dimensions after its mounted.

@toddtarsi
Copy link
Author

EDIT: I don't change the tab label text or the screen / tab size, but I do change the children!

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 18, 2018

I have done some digging, here is what I could profile on a high-end device. I don't understand. I don't see any performance improvement potential.

capture d ecran 2018-06-18 a 23 54 46

capture d ecran 2018-06-18 a 23 51 47

@toddtarsi Do you have a reproduction benchmark where I could see this problem?

@toddtarsi
Copy link
Author

toddtarsi commented Jun 18, 2018

@oliviertassinari - Yeah, I can put something together, but it might take me a bit this week. I think the forced reflow kind of scales in complexity with the DOM.

Also, thanks for getting back to me and testing. I appreciate the swiftness and will do what I can to provide some data.

@oliviertassinari
Copy link
Member

@toddtarsi I would love that! Maybe a codesandbox would be enough.

@toddtarsi
Copy link
Author

@oliviertassinari - I just wanted to say that I won't be able to do anything substantial here after all this week or weekend, but I will next week. Family vacation is this weekend, so I'm getting ready to vanish. 😄

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 6, 2018

@toddtarsi Investigating a slowness issue on our product, I have noticed the same issue:

capture d ecran 2018-07-06 a 18 58 55

However, it only accounts for ~10% of the slowness I'm experiencing with a specific page. It's not a huge priority.

@jedwards1211
Copy link
Contributor

jedwards1211 commented Aug 29, 2018

One of our apps is having this issue but it seems to be pretty much locking up the UI. (Maybe because it affects other layout-responsive components?) We're thinking we'll probably use something besides tabs for unrelated reasons, but we may dig into the issue at some point.

@oliviertassinari
Copy link
Member

The checkTextWrap() logic of the tabs was removed in v4. It should be fine now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tabs This is the name of the generic UI component, not the React module! performance
Projects
None yet
Development

No branches or pull requests

4 participants