Skip to content

[charts] improve tick rendering performance #17755

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

bernardobelchior
Copy link
Member

Improve tick rendering performance.

This improvement is especially visible when zooming in because the tickNumber would grow to large numbers (> 100k) but most of them would be outside the drawing area. Nevertheless, we were still mapping over all of those and creating a huge array. Now, we iterate over the created ticks but only create the tick object for those that will be inside the drawing area.

I think this can be further improved by adjusting the tick number to be slower in the first place.

@bernardobelchior bernardobelchior added performance enhancement This is not a bug, nor a new feature component: charts This is the name of the generic UI component, not the React module! labels May 8, 2025
@bernardobelchior bernardobelchior force-pushed the improve-tick-rendering-perf branch 2 times, most recently from c95783c to 7275f25 Compare May 8, 2025 16:41
@mui-bot
Copy link

mui-bot commented May 8, 2025

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

Generated by 🚫 dangerJS against a990381

@bernardobelchior bernardobelchior force-pushed the improve-tick-rendering-perf branch 3 times, most recently from 7d8970a to 70305d9 Compare May 9, 2025 11:03
Copy link

github-actions bot commented May 9, 2025

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

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 9, 2025
@bernardobelchior bernardobelchior force-pushed the improve-tick-rendering-perf branch from 70305d9 to d9a86b1 Compare May 9, 2025 13:08
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 9, 2025
@@ -44,4 +44,35 @@ describe('ScatterChartPro', () => {
},
options,
);

bench(
Copy link
Member Author

Choose a reason for hiding this comment

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

Added here.

@bernardobelchior bernardobelchior marked this pull request as ready for review May 9, 2025 13:44
@bernardobelchior bernardobelchior requested review from JCQuintas and alexfauquette and removed request for JCQuintas May 9, 2025 13:44
for (let i = 0; i < ticks.length; i += 1) {
const value = ticks[i];
const offset = scale(value);
const point = direction === 'x' ? { x: offset, y: 0 } : { x: 0, y: offset };
Copy link
Member

Choose a reason for hiding this comment

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

the direction in isPointInside handles that for us

Suggested change
const point = direction === 'x' ? { x: offset, y: 0 } : { x: 0, y: offset };
const point = { x: offset, y: offset }

@bernardobelchior bernardobelchior force-pushed the improve-tick-rendering-perf branch from 3e7766e to 20b98ad Compare May 9, 2025 17:19
@mui mui deleted a comment from codspeed-hq bot May 9, 2025
Copy link

codspeed-hq bot commented May 9, 2025

CodSpeed Performance Report

Merging #17755 will improve performances by 9.58%

Comparing bernardobelchior:improve-tick-rendering-perf (a990381) with master (a4aa49e)

Summary

⚡ 1 improvements
✅ 7 untouched benchmarks
🆕 1 new benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
LineChartPro with big data amount 96.3 ms 87.9 ms +9.58%
🆕 ScatterChartPro with big data amount and zoomed in N/A 71.8 ms N/A

@bernardobelchior
Copy link
Member Author

CodSpeed doesn't work well with rebases, but according to the PR the benchmark was introduced in, there is a reduction from 7.5s to 71.8ms, a 100x improvement.

@bernardobelchior bernardobelchior requested a review from JCQuintas May 9, 2025 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants