Skip to content

Conversation

@bernardobelchior
Copy link
Member

@bernardobelchior bernardobelchior commented Sep 18, 2025

Part of #12960.

Cherry-picked from #18941.

This implementation is basically a trade-off. We're moving the Flatbush creation to the start which allows zooming without having to re-create. This contrasts with the current implementation where the Delaunay is created whenever zoom stops interacting.

Even if we're frontloading the computation, for 10k data points the Flatbush implementation is still faster: the whole render takes 480ms vs 600ms with a 6x CPU slowdown. For 2k and 1k data points that Flatbush implementation remains faster.

Regarding the CodSpeed regressions: it's normal that this happens in this use case. The Flatbush is indexing all data points so that it doesn't need to re-index them when the drawing area changes. The Delaunay only indexes visible points. This means that in the regressed examples, where zoom span is small, Delaunay is faster because checking isPointInside is quicker than indexing using Flatbush. That's why the regression exists.

Assuming that most charts will start zoomed out, I think this is an acceptable regression. @alexfauquette @JCQuintas let me know what you think.

@mui-bot
Copy link

mui-bot commented Sep 18, 2025

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

Updated pages:

Bundle size report

Bundle Parsed size Gzip size
@mui/x-data-grid 0B(0.00%) 0B(0.00%)
@mui/x-data-grid-pro 0B(0.00%) 0B(0.00%)
@mui/x-data-grid-premium 0B(0.00%) 0B(0.00%)
@mui/x-charts ▼-11.6KB(-3.50%) ▼-4.34KB(-4.25%)
@mui/x-charts-pro ▼-11.6KB(-2.72%) ▼-4.88KB(-3.67%)
@mui/x-charts-premium ▼-11.6KB(-2.71%) ▼-4.9KB(-3.70%)
@mui/x-date-pickers 0B(0.00%) 0B(0.00%)
@mui/x-date-pickers-pro 0B(0.00%) 0B(0.00%)
@mui/x-tree-view 0B(0.00%) 0B(0.00%)
@mui/x-tree-view-pro 0B(0.00%) 0B(0.00%)

Details of bundle changes

Generated by 🚫 dangerJS against 8776e41

@bernardobelchior bernardobelchior added performance type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. scope: charts Changes related to the charts. labels Sep 18, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Sep 18, 2025

CodSpeed Instrumentation Performance Report

Merging #19615 will degrade performances by 21.92%

Comparing bernardobelchior:flatbush-closest-point (8776e41) with master (4093fe0)1

Summary

⚡ 1 improvement
❌ 2 regressions
✅ 9 untouched

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
ScatterChartPro with big data amount and zoomed in (single renderer) 56.9 ms 68.9 ms -17.52%
ScatterChartPro with big data amount (batch renderer) 71.9 ms 65.3 ms +10.04%
ScatterChartPro with big data amount and zoomed in (batch renderer) 57.5 ms 73.7 ms -21.92%

Footnotes

  1. No successful run was found on master (0f52735) during the generation of this report, so 4093fe0 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@bernardobelchior bernardobelchior force-pushed the flatbush-closest-point branch 3 times, most recently from c8c57d5 to 5b6526f Compare September 19, 2025 12:37
@bernardobelchior bernardobelchior marked this pull request as ready for review September 19, 2025 14:19
defaultXAxisId,
defaultYAxisId,
) {
// FIXME: Do we want to support non-scatter series here?
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to use this to calculate closest bar/line/point based on mouse position for other charts? (for tooltip for example)

Copy link
Member Author

@bernardobelchior bernardobelchior Sep 19, 2025

Choose a reason for hiding this comment

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

Point, yes. Bar probably. Line I don't think so. We might need a separate algorithm or adapt this one.

Copy link
Member

Choose a reason for hiding this comment

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

We can divide a line in multiple points (for better precision), same for bar, the closest point would still be the closest bar/line is most cases no?

Copy link
Member

Choose a reason for hiding this comment

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

There is no need for such a complex data structure for the line/bar charts

Those algorithms are designed for 2D points locations because you can not sort 2D points in a meaning full way.

But line and bar charts have a simple x-axis

  • For bar chart, you get with O(1) the band index according to the x position. That's already what we do for axis highlight
  • For line chart, if points are sorted you can do a quick search, get the closest dataIndex and then loop on the different value associated to this index

Copy link
Member

@JCQuintas JCQuintas left a comment

Choose a reason for hiding this comment

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

I'm ok with the perf changes on zoomed if it means an overall perf improvement

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Sep 22, 2025
@github-actions
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 Sep 22, 2025
@@ -0,0 +1,274 @@
/* eslint-disable no-underscore-dangle,no-plusplus */
Copy link
Member Author

Choose a reason for hiding this comment

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

- Overriding the `marker` slot is not supported;
- Transparent highlight style: for performance reasons, the highlighted state creates a highlighted circle on top of the original marker. Applying transparency to the highlighted circle can cause the original circle to be partially visible.
- Transparent highlight style: for performance reasons, the highlighted state creates a highlighted circle on top of the original marker. Applying transparency to the highlighted circle can cause the original circle to be partially visible;
- `disableHover` for scatter series does not work.
Copy link
Member Author

Choose a reason for hiding this comment

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

disableHover still works when using the svg-single renderer. It just isn't implemented for the svg-batch renderer.

@alexfauquette
Copy link
Member

This contrasts with the current implementation where the Delaunay is created whenever zoom stops interacting.

I'm sorry but it seems this implementation does the same 🫣

I placed a console.log in the flatbrush contructor and when I stop zooming, the constructor runs

I think the issue comes from selectorChartSeriesFlatbushMap which has selectorChartXScales and selectorChartYScales as dependencies.

Those selectors are too far in the processing pipeline. They already take into account the zoom. SO each time you zoom in/out the scales get updated an you need to recompute your flatbrush

I assume what you would need is to divide the creation of scales in a rawScale & zoomedScale such that the flatbrush can only rely on the raw one and so be update only when axes are modified with new domain/range

@bernardobelchior
Copy link
Member Author

I'm sorry but it seems this implementation does the same 🫣

Yes, I'm working on fixing it. I'll need to re-organize the selectors.

I assume what you would need is to divide the creation of scales in a rawScale & zoomedScale

Yes, that's what I'll have to do.

@bernardobelchior
Copy link
Member Author

The issue is that the normalized scale's domain indirectly depends on the zoom data because of zoomFilter: 'discard'. When that option is enabled, the domain of an x-axis can be affected by the zoom data of an y-axis (and vice-versa).

Here's the dependency graph:

image

Since we can't break the dependency from the domain to the zoom data in all cases, there will be some use cases where we'll need to recompute the scales when zoom data changes. I'll try to minimize those cases, but we won't be able to get rid of them.

This means that probably a chart with many data points and zoomFilter: 'discard' will have lower performance than a similar chart without zoom filtering.

@bernardobelchior bernardobelchior marked this pull request as draft September 23, 2025 09:13
@github-actions
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 Sep 29, 2025
@bernardobelchior
Copy link
Member Author

@alexfauquette your suggestion paid off! Thanks 😄

Implementation is here. I'll update this PR once that one is merged.

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

github-actions bot commented Oct 6, 2025

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

@bernardobelchior
Copy link
Member Author

Superseded by #19790

@bernardobelchior bernardobelchior deleted the flatbush-closest-point branch October 8, 2025 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR: out-of-date The pull request has merge conflicts and can't be merged. scope: charts Changes related to the charts. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants