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

[charts] Update Popper position outside of react #15003

Merged
merged 6 commits into from
Oct 28, 2024

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Oct 17, 2024

Part of #14746

The issue seems to come from the onMouseMove which trigger at high frequency costly functions:

  • Updating the tooltip
  • Updating the context that tracks interaction.

In this PR I initially proposed to throttle those updates. 200ms for the interaction, because highlight can suffer a little delay without issue. And 50ms for the tooltip since it's reaction needs to be smoother.

We could gain additional performances by going from 50ms to 200ms for the tooltip, and smoothening it with transition but it creates a weird effect of the tooltip following your mouse like a ghost

After the first feedback, this PR is modified to only move tooltip placement from react state to vanilla JS update.

The management of the context state remains to do

@alexfauquette alexfauquette added bug 🐛 Something doesn't work performance component: charts This is the name of the generic UI component, not the React module! labels Oct 17, 2024
@mui-bot
Copy link

mui-bot commented Oct 17, 2024

Deploy preview: https://deploy-preview-15003--material-ui-x.netlify.app/

Updated pages:

Generated by 🚫 dangerJS against fd0d0bf

Copy link

codspeed-hq bot commented Oct 17, 2024

CodSpeed Performance Report

Merging #15003 will not alter performance

Comparing alexfauquette:charts-cpu (fd0d0bf) with master (ab4823d)

Summary

✅ 3 untouched benchmarks

@romgrk
Copy link
Contributor

romgrk commented Oct 17, 2024

This is an area where you would benefit from doing things outside of react. The tooltip position update could easily be a vanilla JS event handler that updates its position without going through react, the same way we use popperjs in other parts of the codebase. That would allow for high-frequency updates without the UX degradation.

@@ -74,13 +75,16 @@ export function useMouseTracker(): UseMouseTrackerReturnValue {
});
};

const throttledHandleMove = throttle(handleMove, 50);
Copy link
Member

@oliviertassinari oliviertassinari Oct 17, 2024

Choose a reason for hiding this comment

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

50ms, so 20 Hz. The PR preview feels very lagging on my screen. I think we can't go lower than 120 Hz, so 8ms maximum.

Copy link
Contributor

Choose a reason for hiding this comment

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

And tbh even with no throttling there is usually some base amount of lag, because the OS compositor draws the mouse as the last step just before redrawing the monitor, to make the mouse feel as responsive as possible. Meanwhile apps have access to a less recent mouse position. So anything that doesn't go through the native OS APIs will always have some lag.

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.

This is an area where you would benefit from doing things outside of react. The tooltip position update could easily be a vanilla JS event handler that updates its position without going through react, the same way we use popperjs in other parts of the codebase.

💯 to go in this direction. Throttle sounds wrong in the first place. For example, https://mui.com/material-ui/react-tooltip/#follow-cursor is fast enough comparatively, with a x6 CPU slow-down, so I don't think we need this.

I had a bit of a closer look, what I see:

  1. Context propagation. We group different values in a context value, we update some of the context, we don't need the rest of the context, but React is blind to it, it forces a rerender for any part of the context change, e.g. with InteractionContext. It sounds similar to the work that @flaviendelangle is doing on the tree view to isolate state changes. We need something like the grid has, or [data grid] Performance: event-based reactivity #12405.

  2. Unnecessary re-renders. The useMouseTracker() + generateVirtualElement() has the right primitives but the implementation forces a lot of unnecessary re-renders, we should be able to easily refactor this.

With those two, I would expect we are back to 🚀.


// Corresponds to 10 frames at 60 Hz.
// A few bytes payload overhead when lodash/debounce is ~3 kB and debounce ~300 B.
export default function throttle<T extends (...args: any[]) => any>(func: T, wait = 166) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess it duplicate with packages/x-internals/src/throttle/throttle.ts

@alexfauquette alexfauquette changed the title [charts] Throttle mouseMove event to save CPU [charts] Update Popper position outside of react Oct 18, 2024
export function CustomAxisTooltip() {
const tooltipData = useAxisTooltip();
const mousePosition = useMouseTracker(); // Track the mouse position on chart.
Copy link
Member

@oliviertassinari oliviertassinari Oct 19, 2024

Choose a reason for hiding this comment

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

I guess we could have kept the useMouseTracker() function abstraction. The problem seems to be that with anchorEl={generateVirtualElement(mousePosition)}, it returns a new reference at each render. So returning a stable function getter + spreading a popperRef so useMouseTracker() can call popperRef.current.update(); could do it? I mean, we use mousePosition.height and mousePosition.pointerType but those values change very slowly? Maybe event.height can update faster https://w3c.github.io/pointerevents/#dom-pointerevent-height but we don't need > 1 Hz.

I'm raising this to see if we can find way to abstract this logic more between components, to avoid some level of code duplication.

Copy link
Member Author

Choose a reason for hiding this comment

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

The initial idea of useMouseTracker was to provide an abstraction that could help devs whatever the tootlip they use:

The hook was returning the pointer position and its type. Then up to the user to place the tooltip.

The issue of such a hook is that not all library have this notion of .update() for example floating-ui seems to use a hook to update coordinates

The height and pointer type are not an issue. It's mostly to distinguish mousse/fingers so we only measure it during the first interaction. That's why I still save them in a state

const pointerType = usePointerType();

const popperRef: PopperProps['popperRef'] = React.useRef(null);
const virtualElement = React.useRef(generateVirtualElement({ x: 0, y: 0 }));
Copy link
Contributor

Choose a reason for hiding this comment

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

For these type of cases the useLazyRef hook from the @mui/utils is useful. It allows to separate initialization and rendering, so the generateVirtualElement({ ... }) doesn't need to run at every render (creates GC pressure for no reason).

Something like this:

function createVirtualElement() {
  return generateVirtualElement({ x: 0, y: 0 })
}

// then here
const virtualElement = useLazyRef(createVirtualElement)

Copy link
Member Author

Choose a reason for hiding this comment

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

I added it. But the react compiler seems to do not hunderstand that useLazyRef returns a ref. So I had to do the lazy implemetation directly

Copy link
Contributor

Choose a reason for hiding this comment

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

The react compiler or eslint? From what I read on the compiler I would have expected it to be smart enough for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

The eslint(react-compiler/react-compiler)

When I do virtualElement.current = ....

Mutating a value returned from a function whose return value should not be mutated eslint(react-compiler/react-compiler)

And when I don't put virtualElement in the dependency array of the useEffect

React Hook React.useEffect has a missing dependency: 'virtualElement'. Either include it or remove the dependency array.eslintreact-hooks/exhaustive-deps

For the first one, I could disable eslint on the line.
For the second one, I just realized I could add the virtualElement to let dependency array since it is a ref it will never trigger the effect

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 24, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 25, 2024
@alexfauquette alexfauquette merged commit 5f87919 into mui:master Oct 28, 2024
19 checks passed
@alexfauquette alexfauquette deleted the charts-cpu branch October 28, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: charts This is the name of the generic UI component, not the React module! performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants