material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[tooltip] Preventing from unnecessarily rerendering the children component

Open samhuk opened this issue 1 year ago • 6 comments

Summary

This RFC concerns the React Tooltip component and potential performance issues.

Problem

The React Tooltip component unnecessarily rerenders the provided children component ~2-3 times on initial Tooltip render and every time the tooltip's visibility state toggles.

Cause

There are a considerable number of code paths leading up to the final cloneElement call here that create a large number of new references. This leads to the observed unnecessary rerendering behavior.

Experimentation

I have tested this and confirmed that Tooltip indeed causes the children component to unnecessarily render at least 2-3 additional times.

As a PoC, I have also tested with a reduced-complexity (but-otherwise like-for-like) Tooltip component that proves that leveraging React memoization utils eliminates the problem. For example, the ultimate cloneElement call ends up looking something like this:

image

I do not however possess the resources to delve any deeper into this (e.g. in-depth perf benchmarks). Apologies.

Open Questions

  1. Would the (likely significant) refactors be worth-it?
  2. Would the performance gain of less children renders outweigh the performance loss of memoization?

What are the requirements?

Zero unnecessary rerenders of the provided children component.

What are our options?

With my current understanding, there is only one accepted way to achieve this - refactor Tooltip to use React memoization utils to prevent childrenProps from being a new reference or containing new references.

Proposed solution

The crux of the solution is to extract out the cloneElement call and place it inside a useMemo call with the properties of childrenProps as the memoization dependencies.

This may require some additional uses of useCallback in order to ensure the event handler functions such as handleEnter and onMouseEnter are not new references.

Resources and benchmarks

N/A

Search keywords: tooltip, react, performance, memoization

samhuk avatar Feb 17 '24 12:02 samhuk

Assignment moved to Michal per https://www.notion.so/mui-org/component-Tooltip-4764ddf0984c438f8b089decbe868e2e

oliviertassinari avatar Feb 18 '24 13:02 oliviertassinari

The Tooltip is being reimplemented in Base UI. @atomiks, could you take this issue into consideration when working on the new version?

michaldudak avatar Apr 10 '24 19:04 michaldudak

@samhuk just curious if this is actually causing a performance issue for you, or if it's just something technical to note in theory?

For performance, React has a built-in optimization for the children prop. The ExpensiveComponent won't re-render even if CheapComponent does in the following scenario:

<Tooltip title="Delete">
  <CheapComponent>
    <ExpensiveComponent />
  </CheapComponent>
</Tooltip>

You can ensure the direct child is a cheap node, while expensive children are either passed externally like in this example, or memo'd in the CheapComponent's render/return call. In the vast majority of cases though, 1 or 2 extra re-renders is not something to worry about, so that's why I think in this scenario for simple anchor components, it doesn't need investigating.

atomiks avatar Apr 11 '24 04:04 atomiks

Thank you for the response folks.


if this is actually causing a performance issue

I have done some testing and verified that the child node renders 2-3 times. When there are many on the page, it's noticable (when I just shim out Tooltip to a no-op component, the rendering finish times are noticeably lower)


Regarding the Tooltip -> CheapComp -> ExpensiveComp (with no props) nesting and/or memo workaround(s) - I'm sorry, however I don't see how this is relevant or appropriate here. One would hope that a developer knows about the workarounds, providing appropriate dependencies to their elaborate hooks, and such things, however this is not often real.

That being said, this is not high priority, so I am open to this being closed, particularly since it is being reimplemented according to @michaldudak. I don't want to create extra work 🙏

samhuk avatar Apr 14 '24 09:04 samhuk

I have done some testing and verified that the child node renders 2-3 times. When there are many on the page, it's noticable (when I just shim out Tooltip to a no-op component, the rendering finish times are noticeably lower)

I understand that it's rendering multiple times, it's just that this shouldn't be a problem. Instead of shimming out the entire Tooltip component, can you shim out whatever the child anchor is to a plain <span />? Since the crux of this issue is that the child component is being re-rendered multiple times, what's the difference if it's only a plain DOM node? If there's a difference here, that must mean the anchor component you're rendering is expensive? And concretely, what do you mean by "noticeable" %-wise?

This issue is still relevant since the re-implementation has the same "extra" re-renders (in order to set the anchor element state, along with props, and even more when enabling grouping and transitions). However, I haven't had any issues filed about these extra re-renders being a problem. In my experience, slow renders are a problem, not extra re-renders.

atomiks avatar Apr 15 '24 07:04 atomiks

For context, one of the projects I am on is a typical mid-enterpise-size web app, where there is Tooltip on all of the typical web app components plus, for some pages, on almost all cells within some very large MUI DataGridPros.

Some of the pages end up with ~300-500 Tooltip instances. It easily reaches ~600 rerenders (300 * 2 = 600 best case, up to 500 * 3 = 1500 worst case), and, more consequentially, a considerable number of expensive hooks being reevaluated (e.g. grabbing state from URL, session state, doing API requests, DOM manip, etc.).

Percentage-wise, I've managed to estimate it at around a 5-10% increase in page load times for a page without a chunky tooltip-for-each-cell table, and 20-30% otherwise. It really is dependant on the app and the engineering habits of the project.

I understand that it's rendering multiple times, it's just that this shouldn't be a problem

Doesn't this require accepting either the assumption that all Tooltip usage is around simple lightweight elements, or the assumption that all developers are aware of the workarounds necessary to avoid expensive rerenders? My personal view from experience is that neither of these are true. Developers use Tooltip as they see fit, which is usually just <Tooltip><WhateverTheyWant/></Tooltip>. Perhaps I could have better elucidated this versus "I'm sorry, however I don't see how this is relevant or appropriate here" from my previous message. Apologies.

However, I haven't had any issues filed about these extra re-renders being a problem

To be honest, I'm surprised that nobody noticed the double or triple render of their components that are wrapped with Tooltip 😅


Moving forward

To me, I see two possible courses of action:

  1. We close this issue due to it being insufficiently consequential.
  2. The reimplementers of Tooltip do some digging and weigh up the benefit of less rerenders against the cost of adding in the necessary hooks that prevent those rerenders.

samhuk avatar Apr 23 '24 21:04 samhuk

I've also reached this issue for performance problem.

The main issue was caused by <Tooltip /> component.

We used to hamburger menu icon for show menu list, but some how related components are more and more the button reflect really slow.

<Tooltip
  title="menu"
>
  <IconButton onClick={handleHambugerMenuOpen} size="large">
    {hamburgerMenuOpened ? (
      <HamburgerMenuCloseIcon />
    ) : (
      <HamburgerMenuIcon />
    )}
  </IconButton>
</Tooltip>

We test it to wrap <Tooltip /> as memo, but it does not got efficiently. So we decide it to hide tooltip on touch devices.

export const Tooltip = React.memo(function Tooltip({
  isArrow = false,
  sx = [],
  children,
  ...rest
}: TooltipProps & { isArrow?: boolean }) {
  const isTouchDevice = useMediaQuery('(pointer: coarse)');

  if (isTouchDevice) {
    return children;
  }
  
  return (
    <MuiTooltip
      arrow={isArrow}
      componentsProps={{
        tooltip: {
  
  ...

The performance issue is seem to more revealed in mobile(touch) devices.


p.s. We'd got issue when get 60~80 tooltips in same page.

tolluset avatar Jun 04 '24 04:06 tolluset