[charts] Plugin selector memoization issue

[JCQuintas]

Ensure you open a page with few charts when running locally it or it will slow down your browser a lot

The issue mainly seems to revolve around selectors/hooks that accept objects, eg: useItemHighlighted({ seriesId })

And selectors that return a new object, eg:

selectorChartContainerSize = createSelector(
  selectorChartDimensionsState,
  (dimensionsState) => ({
    width: dimensionsState.width + dimensionsState.left + dimensionsState.right,
    height: dimensionsState.height + dimensionsState.top + dimensionsState.bottom,
  }),
);

[mui-bot]

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

Generated by 🚫 dangerJS against 38421c6

[JCQuintas]

A possible solution is to just use fastShallowCompare everywhere. It should fixes all the issues we are seeing.

Another way is to use process.env.NODE_ENV !== 'production' ? complainIfNoMemoize() : Object.is and we will have to manually add the correct comparison to useSelector. Though this only fixes result check, not the prop check

[codspeed-hq]

CodSpeed Performance Report

Merging #16575 will not alter performance

_{Comparing JCQuintas:plugin-selector-memoization-issue (38421c6) with master (2dc2864)}

Summary

✅ 6 untouched benchmarks

[bernardobelchior]

Looks good to me 👍

Not approving because there's some commented code that probably should be used or removed

[bernardobelchior]

Should we use this? I guess it makes sense. Is there any way to remove code when building for a release?

[romgrk]

equalityCheck: process.env.NODE_ENV !== 'production' ? compain : fast

[JCQuintas]

This PR is mostly for discussion on the issue, it does not depict the final solution. 😄

I think the discussion would be more if we want to use fastShallowCompare directly, which would fix the problems everywhere.

Or if we want to have this complain function, and manually fix those when we detect them.

[romgrk]

This logic is a bit confusing to me, why is it checking a for array and b for object?

Also the object check should probably be typeof x === 'object' && x !== null. instanceof Object will fail for Object.create(null), which is required in some cases (e.g. inserting entries by id in an object where the id is a string provided by the user - otherwise ids like 'constructor' or '__proto__' could mess up things).

In general I wouldn't advise using the "fast" methods as generic equality check functions, the tradeoff for "fast" is usually correctness in edge-cases. Imo we should maybe even mark those functions as unsafe_fastObjectShallowCompare to clearly indicate there are invariants that must be taken care of by the caller.

[JCQuintas]

This logic is a bit confusing to me, why is it checking a for array and b for object?

Mistake

In general I wouldn't advise using the "fast" methods as generic equality check functions, the tradeoff for "fast" is usually correctness in edge-cases. Imo we should maybe even mark those functions as unsafe_fastObjectShallowCompare to clearly indicate there are invariants that must be taken care of by the caller.

Should we use a "complete compare" instead?

[romgrk]

Depends on the use-case. Maybe I should have written more in the jsdoc, but the fast object compare is only really meant for record objects like { a: 1, b: 2, ... }, and will possibly fail with anything else (e.g. prototype different from Object, a Date, a RegExp, non-enumerable props, etc). If you can guarantee that it will not receive non-exotic objects, then it's safe to use, but otherwise you should indeed use a "complete compare" focused on robustness/correctness.

[romgrk]

Also note that when I used fastObjectShallowCompare for our selectors in the grid, it was to deal with mutable records that were kept in a ref - in other words, it was a workaround for a hack. Moving forward, I plan that we don't use it anymore for selectors.

Imho, selectors should receive standard arguments, not bag-of-data arguments like { seriesId, etc }, then the standard args memoization logic will work. It will also improve performance to avoid those allocations. In the grid we use intermediate selectors like @alexfauquette outlined below.

[alexfauquette]

Should we have a two selector contructors like

• createNaiveSelector: the one with just Object.is
• createSelector: the one with fast compare

Because we have different usecases.

For example selectors like state => state.interaction => interaction.axis => axis.x they should not require memoization.

For selectors that accept objects as an argument like (highlightState, item) => isItemHighlighted(highlightState, item)
we could effectively warn user about memoizing the item they pass to the callback. But it's not the ideal place because users are not informed about those selectors. The only thing they see are the hooks.

For selectors doing computation. Like selectorChartContainerSize = (size) => ({ width: size.width + size.left + size.right, height: size.height + size.top + size.bottom }) we could either

• use the fast compare to memoize the result
• create intermediate selectors (one for width and one for height) before merging them.

For the second (following code block) I assuem the fact that the selected height and width are the same according to Object.is will be enough to do not run the selectorChartContainerSize and so use the memoized result.

selectorChartContainerWidth = (size) => size.width + size.left + size.right
selectorChartContainerHeight = (size) => height: size.height + size.top + size.bottom

selectorChartContainerSize = createSelector(
  [selectorChartContainerWidth, selectorChartContainerHeight],
  (width, height) => ({ width, height})
)

[JCQuintas]

Should we have a two selector contructors like

• createNaiveSelector: the one with just Object.is
• createSelector: the one with fast compare

I think we have plenty of use-cases. Generally returning an object that is not memoized is not an issue because we have other selectors up/down the road that fixes it somehow.

But any selector that accepts an object will probably not memoize the user's input. Which also means the cache will grow and never get cleaned.

• use the fast compare to memoize the result
• create intermediate selectors (one for width and one for height) before merging them.

This can create quite a lot of boilerplate for a code that is already very boilerplaty 🫠
We can easily only memoize the results in these cases as well

[github-actions]

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