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] Plugin selector memoization issue #16575

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion packages/x-charts/src/internals/plugins/utils/selectors.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,30 @@
import { lruMemoize, createSelectorCreator, CreateSelectorFunction } from 'reselect';
import { fastShallowCompare } from '@mui/x-internals/fastShallowCompare';
import { ChartAnyPluginSignature, ChartState, ChartStateCacheKey } from '../models';

const complainIfNoMemoize =
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

(name: string) =>
(a: any, b: any): boolean => {
const is = Object.is(a, b);
const shallow = fastShallowCompare(a, b);
if (is !== shallow) {
console.warn(
`Selector ${name} returned inconsistent results. Is: ${is} Shallow: ${shallow}`,
a,
b,
);
}
return is;
};

const reselectCreateSelector = createSelectorCreator({
memoize: lruMemoize,
memoizeOptions: {
maxSize: 1,
equalityCheck: Object.is,
// equalityCheck: complainIfNoMemoize('prop check'),
equalityCheck: fastShallowCompare,
// resultEqualityCheck: complainIfNoMemoize('result check'),
resultEqualityCheck: fastShallowCompare,
},
});

Expand Down
13 changes: 12 additions & 1 deletion packages/x-charts/src/internals/store/useSelector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,25 @@ import { useSyncExternalStoreWithSelector } from 'use-sync-external-store/with-s
import { ChartAnyPluginSignature, ChartState } from '../plugins/models';
import { ChartsSelector } from '../plugins/utils/selectors';
import { ChartStore } from '../plugins/utils/ChartStore';
import { fastShallowCompare } from '@mui/x-internals/fastShallowCompare';

const defaultCompare = Object.is;

const complainIfNoMemoize = (a: any, b: any): boolean => {
const is = Object.is(a, b);
const shallow = fastShallowCompare(a, b);
if (is !== shallow) {
console.warn(`useSelector returned inconsistent results. Is: ${is} Shallow: ${shallow}`, a, b);
}
return is;
};

export const useSelector = <TSignatures extends readonly ChartAnyPluginSignature[], TArgs, TValue>(
store: ChartStore<TSignatures>,
selector: ChartsSelector<ChartState<TSignatures>, TArgs, TValue>,
args: TArgs = undefined as TArgs,
equals: (a: TValue, b: TValue) => boolean = defaultCompare,
// equals: (a: TValue, b: TValue) => boolean = complainIfNoMemoize,
equals: (a: TValue, b: TValue) => boolean = fastShallowCompare,
): TValue => {
const selectorWithArgs = (state: ChartState<TSignatures>) => selector(state, args);

Expand Down
19 changes: 19 additions & 0 deletions packages/x-internals/src/fastShallowCompare/fastShallowCompare.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { fastArrayCompare } from '../fastArrayCompare';
import { fastObjectShallowCompare } from '../fastObjectShallowCompare';

/**
* Fast shallow compare.
*
* Builds on top of `fastArrayCompare` and `fastObjectShallowCompare` and uses them for their respective types.
*
* @returns true if `a` and `b` are equal.
*/
export function fastShallowCompare(a: any, b: any) {
if (Array.isArray(a)) {
return fastArrayCompare(a, b);
}
if (b instanceof Object) {
return fastObjectShallowCompare(a, b);
}
Comment on lines +12 to +17
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@romgrk romgrk Feb 14, 2025

Choose a reason for hiding this comment

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

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.

return Object.is(a, b);
}
1 change: 1 addition & 0 deletions packages/x-internals/src/fastShallowCompare/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './fastShallowCompare';