[charts] Fix selector behavior affecting datagrid intergration #20083
Conversation
JCQuintas
requested review from
alexfauquette and
bernardobelchior
as code owners
JCQuintas
added
scope: data grid
|
Deploy preview: https://deploy-preview-20083--material-ui-x.netlify.app/ Bundle size report
|
bernardobelchior
left a comment
bernardobelchior
left a comment
There was a problem hiding this comment.
Could you explain what was the root cause for the issue and why this fixes it?
It's a bit hard to understand without more context.
| }, | ||
| { | ||
| memoizeOptions: { | ||
| equalityCheck: isDeepEqual, |
There was a problem hiding this comment.
I think this only works because getNormalizedAxisScale always returns a new scale, making this isDeepEqual unnecessary because it will always return false.
The reason for isDeepEqual in the normalized domain was to avoid recomputing scale selectors if the domain didn't change, so I think this is negating the performance improvements from #19790 since we'll always be re-creating the Flatbush data structure.
There was a problem hiding this comment.
There is resultEqualityCheck and equalityCheck. This is checking the parameters, not the result. The in the original, the selectorChartFilteredXDomains always re-runs regardless of value, then it checks if the result is equal and return it if they are different.
This is kinda useless, since in selectorChartNormalizedXScales, which is the only place selectorChartFilteredXDomains is used, we depend on selectorChartRawXAxis which is pretty much always different.
Our options are to either memoize the parameters of selectorChartNormalizedXScales, at the cost of traversing and checking the scales, or not memoize at all at the cost of re-render.
The general cause seems to be that @arminmeh mentioned in the body of the issue #20082
But I fail to understand why exactly it happens.
It seems that due to the memoization, the incoming xAxis[].data gets desynced from the actual data used in the scaleBand(domain), where the original arrays change, but their content doesn't.
I believe the solution proposed here solves that by syncing the result of the nomalizer selector to retrigger downstream selectors whenever the deep equal of its inputs changes, at the cost of having to deep traverse the axes every time.
There was a problem hiding this comment.
This is checking the parameters, not the result
I didn't realize that, thanks for calling it out. I wonder if it'll end up being more expensive, though, as checking an object of size-2 arrays is probably cheaper than doing a deep equal of all the arguments. Especially since selectorChartRawXAxis can contain the data array in line and bar charts, which can be a large array.
This is kinda useless, since in selectorChartNormalizedXScales, which is the only place selectorChartFilteredXDomains is used, we depend on selectorChartRawXAxis which is pretty much always different.
selectorChartFilteredXDomains indirectly depends on selectorChartZoomMap. The use case for memoizing the result of the filtered domain was to avoid re-creating the normalized scales when the zoom map changed but didn't cause the domains to change. In that case, we don't want to re-create the scales because it'll cause the flatbush to be re-indexed which is somewhat expensive.
When the axis changes, it'll recompute the world, it's true. We need additional selector work to fix that.
The general cause seems to be that @arminmeh mentioned in the body of the issue #20082
But I fail to understand why exactly it happens.
It seems the root cause isn't entirely known. I'll spend some time investigating this to understand the issue
There was a problem hiding this comment.
I think I found the issue and why this fix works.
It seems that the line @arminmeh mentioned (xScale(baseValue)) finds the baseValue within the xScale.domain().
d3's ordinal scale uses internmap to find compare non-primitives, but it relies on valueOf(). For dates, this works because its valueOf() is a unix timestamp, which is a primitive that can be compared. For arrays, however, this doesn't work (['a','b'].valueOf() === ['a','b'].valueOf() returns false).
Our data is an array of arrays because we're using grouping. The initial render works fine because they're the same reference. However, due to domain memoization, whenever a re-render happens, the arrays in data will be different from the arrays in the scale's domains, causing xScale(baseValue) to return an invalid value.
The change from this PR fixes the issue by breaking memoization whenever the axis changes, which causes the reference to be the same, but to be honest this doesn't address the root cause.
To me, the issue at the root of this problem is how d3 is comparing values when they aren't primitives. IMO, that's what we need to fix.
You can reproduce my assertion by adding the following code here:
if (dataIndex === 0) {
console.log(xScale.domain()[0], baseValue, xScale.domain()[0] === baseValue);
}
There was a problem hiding this comment.
One more piece of information that might help
When I was debugging this, I noticed that the data from useBarPlotData is calculated twice when I do the change that breaks the chart.
First time the data seems to be correct, but the second time the x value becomes NaN (reasons mentioned above)
CodSpeed Performance ReportMerging #20083 will not alter performanceComparing Summary
Benchmarks breakdown
Footnotes |
4 participants
Fixes #20082