Skip to content

[data grid] Performance: selectors #18234

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[data grid] Performance: selectors #18234

wants to merge 4 commits into from

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Jun 4, 2025

Import the selectors implementation from base-ui.

@romgrk romgrk added performance scope: data grid Changes or issues related to the data grid product type: enhancement This is not a bug, nor a new feature labels Jun 4, 2025
romgrk
romgrk commented Jun 4, 2025
View reviewed changes
packages/x-data-grid/src/hooks/features/preferencesPanel/gridPreferencePanelSelector.ts
(panel, labelId: string) => {
(panel, labelId: string | undefined) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The typings from the base-ui selector functions are more precise, they can catch more issues.

@mui-bot
Copy link

mui-bot commented Jun 4, 2025 •
edited
Loading

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

Bundle size report

Bundle size will be reported once CircleCI build #553042 finishes.

Generated by 🚫 dangerJS against 4e9b011

romgrk
romgrk commented Jun 4, 2025
View reviewed changes
packages/x-data-grid/src/hooks/features/virtualization/gridVirtualizationSelectors.ts
Comment on lines -64 to -67
(apiRef: RefObject<GridApiCommunity>) =>
apiRef.current.state.virtualization.renderContext.firstColumnIndex,
(apiRef: RefObject<GridApiCommunity>) =>
apiRef.current.state.virtualization.renderContext.lastColumnIndex,
Copy link
Contributor Author

@romgrk romgrk Jun 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The create selector functions only accept state selectors, not apiRef selectors, but these aren't public so I think this is fine.

romgrk
romgrk commented Jun 4, 2025
View reviewed changes
packages/x-data-grid/src/utils/weakMapMemoize.ts Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this in favor of the unwrapIfNeeded logic.

romgrk
romgrk commented Jun 4, 2025
View reviewed changes
packages/x-data-grid/src/utils/baseCreateSelector.ts Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The base-ui selectors support multiple arguments (very efficiently), but we can't because our useGridSelector(api, selector, args?, equals?) wrapper has the equals argument blocking more arguments. We should rework that API at v9, we do need multiple arguments at some places and we use inefficient param objects instead. We could add useGridSelectorV9, and an alternative useGridSelectorWithEquals(api, selector, equals, ...args) for the limited cases where we need equals.

Copy link
Member

Choose a reason for hiding this comment

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

Added to #17846

romgrk
romgrk commented Jun 4, 2025
View reviewed changes
arminmeh
arminmeh reviewed Jun 5, 2025
View reviewed changes
@cherniavskii cherniavskii mentioned this pull request Jun 5, 2025
Open
cherniavskii
cherniavskii reviewed Jun 5, 2025
View reviewed changes
Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

I didn't quite get where the performance-related changes are. Could you clarify this?

packages/x-data-grid/src/utils/baseCreateSelector.ts Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Added to #17846

packages/x-data-grid/src/utils/weakMapMemoize.ts Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we use it anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#18234 (comment)

@oliviertassinari oliviertassinari changed the title [datagrid] Performance: selectors [data grid] Performance: selectors Jun 5, 2025
@romgrk
Copy link
Contributor Author

romgrk commented Jun 5, 2025

I didn't quite get where the performance-related changes are. Could you clarify this?

The old implementation calls reselect's createSelector every time the selector args change.

@romgrk
Copy link
Contributor Author

romgrk commented Jun 5, 2025

I didn't quite get where the performance-related changes are. Could you clarify this?

Also, the previous implementation had two map.get(...) calls for each selector call, whereas this one has a single one. The cache has been inlined inside createSelectorMemoized, so each selector instance has its own cache instead of having to reach for a global one.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 6, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 6, 2025

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

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 6, 2025
@romgrk romgrk force-pushed the refactor-selectors branch from f8a512a to 8d62906 Compare June 6, 2025 23:40
@romgrk romgrk force-pushed the refactor-selectors branch from 8d62906 to 8e6196b Compare June 7, 2025 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arminmeh arminmeh arminmeh left review comments

@cherniavskii cherniavskii cherniavskii left review comments

@MBilalShafi MBilalShafi Awaiting requested review from MBilalShafi

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
performance scope: data grid Changes or issues related to the data grid product type: enhancement This is not a bug, nor a new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@romgrk @mui-bot @arminmeh @cherniavskii