[charts] Improve scatter chart pointer move performance #18775
Conversation
| if (!highlightScope || !highlightedItem) { | ||
| return alwaysFalse; | ||
| } |
There was a problem hiding this comment.
This is the important part. Now if highlightScope or highlightedItem aren't defined, we always return the same function, so no re-renders happen.
| if (!highlightScope || !highlightedItem) { | ||
| return alwaysFalse; | ||
| } |
There was a problem hiding this comment.
This is the important part. Now if highlightScope or highlightedItem aren't defined, we always return the same function, so no re-renders happen.
| @@ -91,39 +91,53 @@ function Scatter(props: ScatterProps) { | |||
|
|
|||
| const classes = useUtilityClasses(inClasses); | |||
|
|
|||
| const children = React.useMemo(() => { | |||
There was a problem hiding this comment.
Memoized the children so that if isFaded or isHighlighted (or any of the other props, really) don't change, then we don't re-render the children.
There was a problem hiding this comment.
Shouldn't we memo the component then?
There was a problem hiding this comment.
Do you mean the Scatter component? We can, but the root cause of the re-renders was the useItemHighlightedGetter hook, so memoizing Scatter won't have any effect on this particular case.
It might help in other ones, but I'd be more inclined to memoizing it if there's a concrete case where it would help. Is there a case you're thinking of that you think memoizing the component would help?
There was a problem hiding this comment.
No, I mean the children. The Marker component in this case
There was a problem hiding this comment.
Yeah, you're right. For this to have any effect, we'd either need to memoize ScatterMarker or g. I've reverted this change.
|
Deploy preview: https://deploy-preview-18775--material-ui-x.netlify.app/ Bundle size reportTotal Size Change: 🔺+4.84KB(+0.04%) - Total Gzip Change: 🔺+1.24KB(+0.03%) Show details for 100 more bundles (22 more not shown)@mui/x-charts parsed: 🔺+572B(+0.19%) gzip: 🔺+186B(+0.20%) |
CodSpeed Performance ReportMerging #18775 will not alter performanceComparing Summary
|
...charts/src/internals/plugins/featurePlugins/useChartHighlight/useChartHighlight.selectors.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
I find the inline-functions much worse for readability, while it is only useful in the case of debugging.
Though I'll leave that for @alexfauquette discretion.
I've left comments on the ones that feel most unnecessary
| function handleVoronoiMove(event: CustomEvent<PointerGestureEventData>) { | ||
| gestureHandler(event); | ||
| } | ||
| function handleVoronoiPan(event: CustomEvent<PointerGestureEventData>) { | ||
| gestureHandler(event); | ||
| } | ||
| function handleVoronoiQuickPress(event: CustomEvent<PointerGestureEventData>) { | ||
| gestureHandler(event); | ||
| } |
There was a problem hiding this comment.
The reasoning is the one I described in the PR description:
You might notice I transformed a bunch of anonymous functions into named functions. This helps a lot with performance investigations as the proper function names show up in the flame graph.
I'm ok with reverting those, it just means that I'll need to add them back again when debugging, but it's fine.
| function handleAxisMove(event: CustomEvent<PointerGestureEventData>) { | ||
| gestureHandler(event); | ||
| } | ||
| function handleAxisPan(event: CustomEvent<PointerGestureEventData>) { | ||
| gestureHandler(event); | ||
| } | ||
| function handleAxisQuickPress(event: CustomEvent<PointerGestureEventData>) { | ||
| gestureHandler(event); | ||
| } |
There was a problem hiding this comment.
Same as #18775 (comment)
| @@ -91,39 +91,53 @@ function Scatter(props: ScatterProps) { | |||
|
|
|||
| const classes = useUtilityClasses(inClasses); | |||
|
|
|||
| const children = React.useMemo(() => { | |||
There was a problem hiding this comment.
Shouldn't we memo the component then?
There was a problem hiding this comment.
I'm good with moving to more named functions if that help for perf debugging.
I'm a big arrow function user, mostly because It's the way to write maths on paper. But that's an habit I can modify :)
Thanks for moving the item memoisation outside of this PR, we could have a dedicated one to avoid mixing all the perf improvements in a single one 👍
Improve scatter chart pointer move performance when highlight is off.
You might notice I transformed a bunch of anonymous functions into named functions. This helps a lot with performance investigations as the proper function names show up in the flame graph. Although slightly more verbose, I'd prefer if we could use named functions instead of anonymous ones 🙏
Before (
74f16ba78e85ccfb6672d77ecd224f681e468360):Screen.Recording.2025-07-11.at.16.29.36.mov
After:
Screen.Recording.2025-07-11.at.16.29.50.mov
Code from the video: