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] Improve performance of rendering ticks in x-axis #16536

Merged

Conversation

bernardobelchior
Copy link
Member

@bernardobelchior bernardobelchior commented Feb 11, 2025

Depends on #16548.

Part of #10928.

Measuring the size of text using getBoundingClientRect is expensive, especially when appending elements one by one to the DOM, which is what we're doing at the moment.

Knowing that, this PR focuses on reducing the number of times we actually measure text. In the case of tick labels in the x-axis, this is done by:

  1. Checking whether the center of label would overlap with a previous label (or the minimum gap between labels);
  2. Checking whether the center of the label would be outside the visible area.

Only if these two checks are true do we actually measure the text. This helps us reduce the number of measurements, improving performance for when rendering many labels.

Bear in mind that we cache the measurements, so the impact on performance will be less if we're measuring the same strings repeatedly.

Results

Rendering the following bar chart (from the "BarChartPro" benchmark) in a Vite application using production builds:

image

Before

401 cache misses, around 20ms spent measuring text.

After

99 cache misses, around 7ms spent measuring text.

For this example, this means a 75% reduction of measurements taken and around 65% reduction in time spent measuring.

@bernardobelchior bernardobelchior added the component: charts This is the name of the generic UI component, not the React module! label Feb 11, 2025
Comment on lines 101 to 104
const domResults = { count: 0, time: 0 };
if (typeof window !== 'undefined') {
window.domResults = domResults;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Rudimentary way to check how long we're spending in text measurements.

By running domResults, you should see how many times (count) and how long it took in ms (time) to measure all the text that was measured so far. These values only include the cache misses.

let previousTextLimit = 0;
const direction = reverse ? -1 : 1;
return withDimension.map((item, labelIndex) => {
const { width, offset, labelOffset, height } = item;
const minGap = 8;
Copy link
Member Author

Choose a reason for hiding this comment

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

Assumed a gap of 8px for testing purposes. Seems a reasonable default:

image

@mui-bot
Copy link

mui-bot commented Feb 11, 2025

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

Generated by 🚫 dangerJS against 37c1fbb


currentTextLimit = textPosition - (direction * (gapRatio * distance)) / 2;
if (labelIndex > 0 && direction * currentTextLimit < direction * previousTextLimit) {
if (labelIndex > 0 && direction * (textPosition - minGap) < direction * previousTextLimit) {
Copy link
Member Author

Choose a reason for hiding this comment

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

If there isn't enough space between the end of the last text plus the gap and the center of this tick, then we don't even need to calculate the width because we already know it won't fit.

@bernardobelchior bernardobelchior force-pushed the x-axis-improve-text-measure-perf branch from 0d87312 to bc0ef42 Compare February 11, 2025 13:45
Comment on lines 86 to 93
if (!isPointInside({ x: textPosition, y: -1 }, { direction: 'x' })) {
return { ...item, width: 0, height: 0, skipLabel: true };
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Don't calculate label width if the tick is outside the visible area (useful when zooming)

@bernardobelchior
Copy link
Member Author

The results are still early and may change, but look promising.

These changes reduced the number of actual DOM measurements from 401 to 83, and the time spent measuring from 126ms to 33ms (MacBook Pro M4 Pro, 48GB RAM, 6x slowdown), which is a 3.8x improvement.

This is the component I'm using for benchmarking:

import { BarChartPro } from '@mui/x-charts-pro';

const dataLength = 400;
const data = Array.from({ length: dataLength + 1 }).map((_, i) => ({
  x: i,
  y: 50 + Math.sin(i / 5) * 25,
}));

const xData = data.map((d) => d.x);
const yData = data.map((d) => d.y);

export default function BarChartProBench() {
  return (
    <BarChartPro
      xAxis={[{ id: 'x', scaleType: 'band', data: xData, zoom: { filterMode: 'discard' } }]}
      initialZoom={[{ axisId: 'x', start: 25, end: 75 }]}
      series={[
        {
          data: yData,
        },
      ]}
      width={500}
      height={300}
    />
  );
}

@bernardobelchior bernardobelchior force-pushed the x-axis-improve-text-measure-perf branch from c889253 to 5218dd8 Compare February 11, 2025 15:56
@bernardobelchior bernardobelchior force-pushed the x-axis-improve-text-measure-perf branch from df4e976 to 0b8b73e Compare February 11, 2025 16:47
Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

Looks already pretty solid. I added a commit to fix the benchmark. Your PR modifies which label is visible or not. And it impacted the labels the benchmark test was looking for to compute the rendering duration

Is their something missing to have this as ready for review?

@bernardobelchior
Copy link
Member Author

Looks already pretty solid. I added a commit to fix the benchmark. Your PR modifies which label is visible or not. And it impacted the labels the benchmark test was looking for to compute the rendering duration

Thanks!

Is their something missing to have this as ready for review?

Yeah, I'm experimenting with other approaches. You can see them here and here.

The reason I don't want to merge this yet is because my investigations unveiled a bug in the measurement logic. The default label is being measured as 18px, when it is actually 21px. The gapRatio of 1.2 fixes the issue, but this PR removes that ratio. If someone is using minGap={0}, they might see overlapping labels, so I'd rather fix the issue first, then open this PR for review.

This lead me to investigating how to performantly and accurately measure the width and height of text, which is what I'm exploring in those PoCs. Feel free to give your opinion. I think they're ready for some feedback 😄

@bernardobelchior bernardobelchior force-pushed the x-axis-improve-text-measure-perf branch 2 times, most recently from bda19ce to be9591d Compare February 19, 2025 09:37
@bernardobelchior bernardobelchior removed the request for review from alexfauquette February 19, 2025 10:04
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 20, 2025
Copy link

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

@bernardobelchior bernardobelchior force-pushed the x-axis-improve-text-measure-perf branch from be9591d to 0c490da Compare February 20, 2025 16:03
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 20, 2025
Copy link

codspeed-hq bot commented Feb 20, 2025

CodSpeed Performance Report

Merging #16536 will not alter performance

Comparing bernardobelchior:x-axis-improve-text-measure-perf (37c1fbb) with master (ff10228)

Summary

✅ 6 untouched benchmarks

@bernardobelchior bernardobelchior marked this pull request as ready for review February 20, 2025 16:47
@bernardobelchior bernardobelchior merged commit b949938 into mui:master Feb 21, 2025
19 checks passed
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants