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

[EuiTooltip] Performance issues #3568

Closed
kertal opened this issue Jun 5, 2020 · 12 comments · Fixed by #3596
Closed

[EuiTooltip] Performance issues #3568

kertal opened this issue Jun 5, 2020 · 12 comments · Fixed by #3596

Comments

@kertal
Copy link
Member

kertal commented Jun 5, 2020

Depending on the system EuiTooltip caused a performance issue in Kibana Discover. The attached Gifs show a significant higher CPU usage by hovering the Elements containing a EuiTooltip it in Chrome 83, MacOs 10.14.6. While it wasn't that bad on my MacBook Pro (just when moving around the mouse like crazy), it seems to have a significant higher performance impact on Windows based machines (also when not moving around the mouse quickly), also Linux based system reported a much higher impact. Here is the original issue that reported it:

elastic/kibana#67732

Here the PR replacing EuiTooltip:

elastic/kibana#68280

Sidebar elements with EuiTooltip
image

Sidebar elements with native tooltip with title attribute
image

@snide
Copy link
Contributor

snide commented Jun 5, 2020

I'd like to try and replicate this outside of discover. I'll see if I can make a quick code sandbox example to verify the issue.

FWIW, I think using a title instead of a tooltip is a better UI decision anyway, but lets see where this is getting hung up.

@kertal
Copy link
Member Author

kertal commented Jun 10, 2020

FYI: EuiTooltip in Discover context was triggering a weird jump to top / rerendering without reloading of the Discover in Firefox, when you scrolled down, hovered a fieldname it was jumping like this (also fixed with going native)

image

@kertal
Copy link
Member Author

kertal commented Jun 10, 2020

There's a similiar issue here related to EuiTooltip elastic/kibana#58204, I could now reproduce it:
Discover_-_Elastic_Kibana
One user posted also a solution, which seems to be a simple CSS fix

@-moz-document url-prefix() {
    .euiBody-hasPortalContent {
        position: static !important;
    }
}

@timroes
Copy link
Contributor

timroes commented Jun 10, 2020

@snide Could you clarify if this is a general recommendation not to use EuiTooltip then anymore? Because this performance issue is really severe for us, and if the solution from design is to use title instead, we'd start removing EuiTooltip in large across our apps. So would be good to have some clarity (and maybe adding it to the EUI documentation) if we no longer recommend using this component, or if the issue can somehow be prioritized on your side?

@cchaos
Copy link
Contributor

cchaos commented Jun 10, 2020

@timroes This is certainly not an advocate for not using EuiTooltip. Discover was the first application to uncover such a problem so we should certainly aim to fix it on the EUI side. @snide is already working on triaging it at the EUI side with constructing the Discover implementation.

The advice for using title over EuiTooltips usually has more to do with truncation. We tend to use title if we're simply displaying the full item name if there's a possibility of it being truncated with euiTextTruncate. It is not a replacement for displaying help text. However, we will discuss internally if we need to add some guidelines over the uses of both.

@thompsongl
Copy link
Contributor

Regardless of the EuiTooltip vs. title discussion, I think we could certainly increase EuiTooltip performance by refactoring the component.
Right now, every render and rerender results in a lot of potentially unnecessary calculation and subcomponent mount/unmount.

I can give it a go and see what gains I can get.

@thompsongl
Copy link
Contributor

I can give it a go and see what gains I can get.

I'll keep going with this, but an update for now:
Even without profiling, the visual lag is easily reproducible in the EUI docs by creating a simple component to mimic the show-button-on-hover UI in Kibana.
My thesis was that cloneElement, EuiPortal, and EuiResizeObserver inside EuiTooltip were the source of the slowness, but removing them all from the component has no visual effect.

@chandlerprall
Copy link
Contributor

chandlerprall commented Jun 11, 2020

Did some quick testing as well; it appears that the slowdown is caused from the this.hideToolTip() call, so perhaps removing the portal & the resulting reflow is more expensive than creating it? If that's so, perhaps we need to wait to create the portal & tooltip DOM until that animation is triggered - moving the delay from CSS to JS.

(Greg, please verify that disabling the onMouseOut/hideToolTip resolves for you as well, I'm not 100% sure I set this up right)

@thompsongl
Copy link
Contributor

thompsongl commented Jun 11, 2020

I was able to get good results by using our enqueueStateChange utility on hideToolTip and showTooltip, effectively eliminating the DOM add/remove churn for elements that don't ever need to be rendered.
There doesn't appear to be any negative impact on simpler cases.

@chandlerprall
Copy link
Contributor

Nice! Good thinking!

@thompsongl
Copy link
Contributor

perhaps we need to wait to create the portal & tooltip DOM until that animation is triggered - moving the delay from CSS to JS

This helps a lot, also. Going to incorporate the idea in the open PR.

@thompsongl
Copy link
Contributor

Changes in #3596 result in only single-digit Chrome renderer CPU % increases compared with a stateless, tooltip-less component after excessive mouseover/mouseout activity.
More normal interaction (like in the original gifs above) will result in negligible CPU increases when using the improved EuiTooltip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants