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

[TouchRipple] Avoid calling clearTimeout() to improve performance #37512

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Jun 6, 2023

I've been profiling the X DataGrid as part of mui/mui-x#9037, I've noticed that the clearTimeout() calls coming from TouchRipple.js are consistently taking about 5% of the CPU time for each of our scroll events on blink-based browsers.

I'm guessing clearTimeout crosses the boundary into C++ code, which isn't great for performance. This PR avoids calling the function if no timeout has been started.

I'm guessing the same performance hit applies to all clearTimeout() calls in the codebase, I'm not sure if you'd like to find a more general solution. Let me know if you prefer the change to be applied to all clearTimeout calls.

image

@mui-bot
Copy link

mui-bot commented Jun 6, 2023

Netlify deploy preview

https://deploy-preview-37512--material-ui.netlify.app/

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 5aa720e

@zannager zannager requested a review from mnajdova June 6, 2023 12:44
@romgrk
Copy link
Contributor Author

romgrk commented Jun 19, 2023

Ping @gzrae, not sure who to ping to get feedback here.

@michaldudak michaldudak added the package: material-ui Specific to @mui/material label Jun 21, 2023
@michaldudak michaldudak changed the title perf: avoid calling clearTimeout() [TouchRipple] perf: avoid calling clearTimeout() Jun 21, 2023
Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

Great catch! As for the general fix, we could introduce a wrapper around clearTimeout that checks if a timeout id is valid, but then we'll have to make sure this wrapper is called every time instead of the native function. I suppose adding checks before clearTimeout is more explicit.

@romgrk romgrk merged commit 6167b6a into mui:master Jun 21, 2023
19 of 20 checks passed
@romgrk romgrk deleted the perf-avoid-clear-timeout branch June 21, 2023 08:00
@romgrk
Copy link
Contributor Author

romgrk commented Aug 14, 2023

I've merged in this datagrid PR a wrapper for timeout, it handles this case and doesn't allocate anything after the first render so it's super efficient. We can migrate it to core at some point.

@oliviertassinari oliviertassinari changed the title [TouchRipple] perf: avoid calling clearTimeout() [TouchRipple] Avoid calling clearTimeout() to improve performance Aug 20, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 20, 2023

@romgrk 👌


I created a quick reproduction for this performance issue.

import * as React from 'react';
import Button from '@mui/material/Button';

export default function BasicButtons() {
  const [show, setShow] = React.useState(true);
  return (
    <div>
      <button onClick={() => {
        setShow(false);
      }}>
        Hide
      </button>
      {show ? <div>
        {Array.from(new Array(1000)).map((x, index) => (
          <Button key={index} variant="contained">Contained</Button>
        ))}
        </div>
      : null}
    </div>
  );
}

On a x4 slowdown CPU (relative to my Apple M1 Pro), we are spending 60ms to unmount 1,000 buttons.

Screenshot 2023-08-20 at 22 54 11

It gets really absurd when unmounting Tooltips:

Screenshot 2023-08-20 at 23 06 07
import * as React from 'react';
import Tooltip from '@mui/material/Tooltip';

export default function BasicButtons() {
  const [show, setShow] = React.useState(true);
  return (
    <div>
      <button onClick={() => {
        setShow(false);
      }}>
        Hide
      </button>
      {show ? <div>
        {Array.from(new Array(1000)).map((x, index) => (
          <Tooltip title="hello">
            <button key={index}>Contained</button>
          </Tooltip>
        ))}
        </div>
      : null}
    </div>
  );
}

It looks like a quick-win someone should take on, e.g. maybe with the helper Romain proposed. I created a task for it: https://www.notion.so/mui-org/clearTimeout-safeguard-d469f036aa3b4d49b503fd312863ef5b, but best to move it to GitHub issues (I didn't to avoid getting notifications, but still created the task as it looks too good to get forgotten).

@oliviertassinari oliviertassinari added component: button This is the name of the generic UI component, not the React module! component: ButtonBase The React component. labels Aug 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: button This is the name of the generic UI component, not the React module! component: ButtonBase The React component. package: material-ui Specific to @mui/material performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants