[charts] Filter item outside the drawing area for perf #14281
Conversation
alexfauquette
added
performance
component: charts
|
Deploy preview: https://deploy-preview-14281--material-ui-x.netlify.app/ |
CodSpeed Performance ReportMerging #14281 will not alter performanceComparing Summary
Benchmarks breakdown
|
| @@ -1,5 +1,7 @@ | |||
| import * as React from 'react'; | |||
| import PropTypes from 'prop-types'; | |||
|
|
|||
| import type {} from '../typeOverloads'; | |||
There was a problem hiding this comment.
Otherwise TS was not understading that pro component have zoom options
| if ( | ||
| result.x > xMax || | ||
| result.x + result.width < xMin || | ||
| result.y > yMax || | ||
| result.y + result.height < yMin | ||
| ) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
That, plus the line .filter((rectangle) => rectangle !== null); ar the only modification. in this function. The rest is an indentation diff leading to some prettier modification
alexfauquette
force-pushed
the
filter-with-zoom
branch
from
7f8dbdc to
0c28f3f
Compare
| @@ -24,7 +24,7 @@ describe('ScatterChart', () => { | |||
| async () => { | |||
| const { findByText } = render( | |||
| <ScatterChart | |||
| xAxis={[{ data: xData }]} | |||
| xAxis={[{ data: xData, valueFormatter: (v) => v.toLocaleString('en-US') }]} | |||
There was a problem hiding this comment.
I don't know why but I had an issue on my computer. The default locale of .toLocaleString was not the same in the test and in the render (1 000 vs 1,000)
github-actions
bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
removed
the
PR: out-of-date
This reverts commit b141174.
github-actions
bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
removed
the
PR: out-of-date
| @@ -43,7 +43,7 @@ describe('ScatterChartPro', () => { | |||
| valueFormatter: (v) => v.toLocaleString('en-US'), | |||
| }, | |||
| ]} | |||
| zoom={[{ axisId: 'x', start: 0.25, end: 0.75 }]} | |||
| zoom={[{ axisId: 'x', start: 2, end: 7 }]} | |||
There was a problem hiding this comment.
@JCQuintas does this filter something? From what I understood, the zoom feature you implemented takes a ratio, so 2, 7 will just add lot of white space around the data and then we don't test the impact of removing points which are outside of the drawing area
There was a problem hiding this comment.
/**
* The starting percentage of the zoom range. In the range of 0 to 100.
*
* @default 0
*/
start: number;
/**
* The ending percentage of the zoom range. In the range of 0 to 100.
*
* @default 100
*/
end: number;
There was a problem hiding this comment.
internally we use the ratio, externally is a range from 0...100
There was a problem hiding this comment.
Thanks I forgot we those test have thousands of points and then 0.25% was still significant for other charts 👍
There was a problem hiding this comment.
Yeah I had to remove a lot of datapoints for the CI to work with the Scatter. It still seems flaky, so we might need to adjust, fix or disable it if it continues like this 😓
I noticed that zooming on 5% of the data takes as much time to render than a full zoom out. The more we zoom the faster it should render since less elements are to plot
For Bar charts, zoning on 5% of a 1,000 bars (only shows 50 bars out of the 1,000) makes the rendering time move from 100ms to 28ms