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.

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

[DataGrid] Filtering performance #9120

Closed
wants to merge 24 commits into from
Closed

[DataGrid] Filtering performance #9120

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
2117914
perf: dont do expensive allocations
romgrk May 25, 2023
0b1d56e
perf: fast filters
romgrk May 25, 2023
5409eb7
perf: non-memoized selectors
romgrk May 26, 2023
32620fc
perf: reduced indirection in API methods
romgrk May 26, 2023
19902ee
perf: avoid dynamic object prop
romgrk May 26, 2023
a117b38
perf: direct state access
romgrk May 26, 2023
e222e9a
perf: use Set for rows lookup
romgrk May 26, 2023
efb90cf
perf: remove allocations
romgrk May 28, 2023
bbe66fd
perf: specialize createRawSelector
romgrk May 28, 2023
2fb95ff
lint
romgrk May 28, 2023
a674f20
lint
romgrk May 28, 2023
e687e0d
perf: specialize passFilterLogic
romgrk May 28, 2023
02977be
lint
romgrk May 28, 2023
b917232
lint
romgrk May 28, 2023
65694fb
chore: synchronous demo data
romgrk May 28, 2023
6296c81
perf: new filtering API
romgrk May 29, 2023
460b6e3
perf: new filtering API
romgrk May 29, 2023
b665573
perf: remove API Proxy wrapper
romgrk May 29, 2023
4b5164c
lint
romgrk May 29, 2023
072c659
fix: minor issues
romgrk May 29, 2023
8c6cdec
lint
romgrk May 29, 2023
8183aaa
Revert "chore: synchronous demo data"
romgrk May 29, 2023
aa90c79
lint
romgrk May 29, 2023
189acf0
lint
romgrk Jun 3, 2023
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
2 changes: 1 addition & 1 deletion packages/grid/x-data-grid-premium/src/DataGridPremium/useDataGridPremiumComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ export const useDataGridPremiumComponent = (
useGridRowPinning(privateApiRef, props);
romgrk marked this conversation as resolved.
Show resolved Hide resolved
useGridColumns(privateApiRef, props);
useGridRows(privateApiRef, props);
useGridParamsApi(privateApiRef);
useGridParamsApi(privateApiRef, props);
useGridDetailPanel(privateApiRef, props);
useGridColumnSpanning(privateApiRef);
useGridColumnGrouping(privateApiRef, props);
Expand Down
24 changes: 13 additions & 11 deletions packages/grid/x-data-grid-premium/src/hooks/features/rowGrouping/gridRowGroupingUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
import {
Copy link
Member

Choose a reason for hiding this comment

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

Apart from the specific discussions going on, generally speaking, the initiatives taken for the performance enhancement in this PR are really solid, a few of them were actually new to me and they strengthened my basic concepts about how things operate under the hood, so a huge thanks. I think the direction we are going in is outstanding.
I'll really appreciate it if it's possible to extract these changes (as we already discussed) into smaller PRs to:

  1. Have a stronger spotlight on the optimization each of the changes is bringing in
  2. To measure the impact of each change separately (and possibly push it further)
  3. To assess which of the changes could be applicable to the other areas of the application.

1, 2: Points no 1 and 2 are certainly the most impacting ones, although we are going to have a workaround (.v7), but with v7 around the corner and the benefits internally applied, that seems a step forward.
3: Non-cached selector option is a nice improvement too, why need to cache something when it could be accessed with a simple (.) notation.
4: It seems, the indirection (or checking of properties was to stop initializing if already did or have stable references, but I am not sure why it was wrapped inside the register function). It'll be good to simplify it if it doesn't cause a side effect.
5. I have never used eval this way, maybe discuss this one separately and see if there are any loose grounds?
6, 7: These are good, I think we should do them where possible in a non-breakable way and plan for v7 for the remaining part

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, let's continue the ongoing discussions in the threads above, I'll open separate PRs as we reach an agreement for each of those.

passFilterLogic,
GridAggregatedFilterItemApplier,
GridAggregatedFilterItemApplierResult,
GridColumnRawLookup,
GridApiCommunity,
} from '@mui/x-data-grid-pro/internals';
Expand Down Expand Up @@ -83,17 +84,17 @@ export const filterRowTreeFromGroupingColumns = (
params: FilterRowTreeFromTreeDataParams,
): Omit<GridFilterState, 'filterModel'> => {
const { rowTree, isRowMatchingFilters, filterModel } = params;
const visibleRowsLookup: Record<GridRowId, boolean> = {};
const filteredRowsLookup: Record<GridRowId, boolean> = {};
const visibleRowsLookup = new Set<GridRowId>();
const filteredRowsLookup = new Set<GridRowId>();
const filteredDescendantCountLookup: Record<GridRowId, number> = {};

const filterTreeNode = (
node: GridTreeNode,
areAncestorsExpanded: boolean,
ancestorsResults: ReturnType<GridAggregatedFilterItemApplier>[],
ancestorsResults: GridAggregatedFilterItemApplierResult[],
): number => {
let isPassingFiltering = false;
let filterResults: ReturnType<GridAggregatedFilterItemApplier> = {
const filterResults: GridAggregatedFilterItemApplierResult = {
passingFilterItems: null,
passingQuickFilterValues: null,
};
Expand All @@ -104,7 +105,8 @@ export const filterRowTreeFromGroupingColumns = (
? (columnField: string) => shouldApplyFilterItemOnGroup(columnField, node)
: undefined;

filterResults = isRowMatchingFilters(node.id, shouldApplyItem);
const row = params.apiRef.current.getRow(node.id);
isRowMatchingFilters(row, shouldApplyItem, filterResults);
} else {
isPassingFiltering = true;
}
Expand All @@ -117,7 +119,7 @@ export const filterRowTreeFromGroupingColumns = (
childNode,

areAncestorsExpanded && !!node.childrenExpanded,
[...ancestorsResults, filterResults],
[...ancestorsResults, Object.assign({}, filterResults)],
);
filteredDescendantCount += childSubTreeSize;
});
Expand All @@ -128,7 +130,7 @@ export const filterRowTreeFromGroupingColumns = (
// If node has children - it's passing if at least one child passes filters
isPassingFiltering = filteredDescendantCount > 0;
} else {
const allResults = [...ancestorsResults, filterResults];
const allResults = [...ancestorsResults, Object.assign({}, filterResults)];
isPassingFiltering = passFilterLogic(
allResults.map((result) => result.passingFilterItems),
allResults.map((result) => result.passingQuickFilterValues),
Expand All @@ -138,13 +140,13 @@ export const filterRowTreeFromGroupingColumns = (
}
}

visibleRowsLookup[node.id] = isPassingFiltering && areAncestorsExpanded;
filteredRowsLookup[node.id] = isPassingFiltering;
if (isPassingFiltering && areAncestorsExpanded) visibleRowsLookup.add(node.id);
if (isPassingFiltering) filteredRowsLookup.add(node.id);

// TODO rows v6: Should we keep storing the visibility status of footer independently or rely on the group visibility in the selector ?
if (node.type === 'group' && node.footerId != null) {
visibleRowsLookup[node.footerId] =
isPassingFiltering && areAncestorsExpanded && !!node.childrenExpanded;
if (isPassingFiltering && areAncestorsExpanded && !!node.childrenExpanded)
visibleRowsLookup.add(node.footerId);
}

if (!isPassingFiltering) {
Expand Down
2 changes: 1 addition & 1 deletion packages/grid/x-data-grid-pro/src/DataGridPro/useDataGridProComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ export const useDataGridProComponent = (
useGridRowPinning(apiRef, props);
useGridColumns(apiRef, props);
useGridRows(apiRef, props);
useGridParamsApi(apiRef);
useGridParamsApi(apiRef, props);
useGridDetailPanel(apiRef, props);
useGridColumnSpanning(apiRef);
useGridColumnGrouping(apiRef, props);
Expand Down
28 changes: 17 additions & 11 deletions packages/grid/x-data-grid-pro/src/hooks/features/treeData/gridTreeDataUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ import {
} from '@mui/x-data-grid';
import {
GridAggregatedFilterItemApplier,
GridAggregatedFilterItemApplierResult,
GridApiCommunity,
passFilterLogic,
passFilterLogicSingle,
} from '@mui/x-data-grid/internals';

interface FilterRowTreeFromTreeDataParams {
Expand All @@ -30,10 +31,15 @@ export const filterRowTreeFromTreeData = (
params: FilterRowTreeFromTreeDataParams,
): Omit<GridFilterState, 'filterModel'> => {
const { rowTree, disableChildrenFiltering, isRowMatchingFilters } = params;
const visibleRowsLookup: Record<GridRowId, boolean> = {};
const filteredRowsLookup: Record<GridRowId, boolean> = {};
const visibleRowsLookup = new Set<GridRowId>();
const filteredRowsLookup = new Set<GridRowId>();
const filteredDescendantCountLookup: Record<GridRowId, number> = {};

const filterResults: GridAggregatedFilterItemApplierResult = {
passingFilterItems: null,
passingQuickFilterValues: null,
};

const filterTreeNode = (
node: GridTreeNode,
isParentMatchingFilters: boolean,
Expand All @@ -47,10 +53,10 @@ export const filterRowTreeFromTreeData = (
} else if (!isRowMatchingFilters || node.type === 'footer') {
isMatchingFilters = true;
} else {
const { passingFilterItems, passingQuickFilterValues } = isRowMatchingFilters(node.id);
isMatchingFilters = passFilterLogic(
[passingFilterItems],
[passingQuickFilterValues],
isRowMatchingFilters(node.id, undefined, filterResults);
isMatchingFilters = passFilterLogicSingle(
filterResults.passingFilterItems,
filterResults.passingQuickFilterValues,
params.filterModel,
params.apiRef,
);
Expand Down Expand Up @@ -86,13 +92,13 @@ export const filterRowTreeFromTreeData = (
}
}

visibleRowsLookup[node.id] = shouldPassFilters && areAncestorsExpanded;
filteredRowsLookup[node.id] = shouldPassFilters;
if (shouldPassFilters && areAncestorsExpanded) visibleRowsLookup.add(node.id);
if (shouldPassFilters) filteredRowsLookup.add(node.id);

// TODO: Should we keep storing the visibility status of footer independently or rely on the group visibility in the selector ?
if (node.type === 'group' && node.footerId != null) {
visibleRowsLookup[node.footerId] =
shouldPassFilters && areAncestorsExpanded && !!node.childrenExpanded;
if (shouldPassFilters && areAncestorsExpanded && !!node.childrenExpanded)
visibleRowsLookup.add(node.footerId);
}

if (!shouldPassFilters) {
Expand Down
2 changes: 1 addition & 1 deletion packages/grid/x-data-grid/src/DataGrid/useDataGridComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ export const useDataGridComponent = (
useGridRowSelection(privateApiRef, props);
useGridColumns(privateApiRef, props);
useGridRows(privateApiRef, props);
useGridParamsApi(privateApiRef);
useGridParamsApi(privateApiRef, props);
useGridColumnSpanning(privateApiRef);
useGridColumnGrouping(privateApiRef, props);
useGridEditing(privateApiRef, props);
Expand Down
4 changes: 2 additions & 2 deletions packages/grid/x-data-grid/src/colDef/gridBooleanOperators.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ import { GridFilterOperator } from '../models/gridFilterOperator';
export const getGridBooleanOperators = (): GridFilterOperator<any, boolean | null, any>[] => [
{
value: 'is',
getApplyFilterFn: (filterItem: GridFilterItem) => {
getApplyFilterFnV7: (filterItem: GridFilterItem) => {
if (!filterItem.value) {
return null;
}

const valueAsBoolean = filterItem.value === 'true';
return ({ value }): boolean => {
return (value, _, __, ___): boolean => {
Copy link
Member

Choose a reason for hiding this comment

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

Why not skip the unused arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old habit, V8 used to have an adaptor for functions with mismatched arity that was expensive, but the overhead is mostly gone now. But I still like to write javascript code that is easy & predictable for engines to optimize. In particular for functions like this one that are run in a hot loop.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the article, I have learned a lot about v8 lately 🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Np, I love learning about JS engines internals, helps a lot with performance optimization. I'm less familiar with SpiderMonkey though, and I know very little about JSC. But V8 is the most common engine by far.

If you're interested in reading more, those links are all interesting:

return Boolean(value) === valueAsBoolean;
};
},
Expand Down
27 changes: 13 additions & 14 deletions packages/grid/x-data-grid/src/colDef/gridDateOperators.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { GridFilterInputDate } from '../components/panel/filterPanel/GridFilterInputDate';
import { GridFilterItem } from '../models/gridFilterItem';
import { GridFilterOperator } from '../models/gridFilterOperator';
import { GridCellParams } from '../models/params/gridCellParams';
import { GridFilterOperator, GridApplyFilterV7 } from '../models/gridFilterOperator';

const dateRegex = /(\d+)-(\d+)-(\d+)/;
const dateTimeRegex = /(\d+)-(\d+)-(\d+)T(\d+):(\d+)/;
Expand All @@ -11,7 +10,7 @@ function buildApplyFilterFn(
compareFn: (value1: number, value2: number) => boolean,
showTime?: boolean,
keepHours?: boolean,
) {
): GridApplyFilterV7 | null {
if (!filterItem.value) {
return null;
}
Expand All @@ -23,7 +22,7 @@ function buildApplyFilterFn(

const time = new Date(year, month - 1, day, hour || 0, minute || 0).getTime();

return ({ value }: GridCellParams<any, Date, any>): boolean => {
return (value, _, __, ___): boolean => {
if (!value) {
return false;
}
Expand All @@ -47,39 +46,39 @@ function buildApplyFilterFn(
export const getGridDateOperators = (showTime?: boolean): GridFilterOperator<any, Date, any>[] => [
{
value: 'is',
getApplyFilterFn: (filterItem) => {
getApplyFilterFnV7: (filterItem) => {
return buildApplyFilterFn(filterItem, (value1, value2) => value1 === value2, showTime);
},
InputComponent: GridFilterInputDate,
InputComponentProps: { type: showTime ? 'datetime-local' : 'date' },
},
{
value: 'not',
getApplyFilterFn: (filterItem) => {
getApplyFilterFnV7: (filterItem) => {
return buildApplyFilterFn(filterItem, (value1, value2) => value1 !== value2, showTime);
},
InputComponent: GridFilterInputDate,
InputComponentProps: { type: showTime ? 'datetime-local' : 'date' },
},
{
value: 'after',
getApplyFilterFn: (filterItem) => {
getApplyFilterFnV7: (filterItem) => {
return buildApplyFilterFn(filterItem, (value1, value2) => value1 > value2, showTime);
},
InputComponent: GridFilterInputDate,
InputComponentProps: { type: showTime ? 'datetime-local' : 'date' },
},
{
value: 'onOrAfter',
getApplyFilterFn: (filterItem) => {
getApplyFilterFnV7: (filterItem) => {
return buildApplyFilterFn(filterItem, (value1, value2) => value1 >= value2, showTime);
},
InputComponent: GridFilterInputDate,
InputComponentProps: { type: showTime ? 'datetime-local' : 'date' },
},
{
value: 'before',
getApplyFilterFn: (filterItem) => {
getApplyFilterFnV7: (filterItem) => {
return buildApplyFilterFn(
filterItem,
(value1, value2) => value1 < value2,
Expand All @@ -92,25 +91,25 @@ export const getGridDateOperators = (showTime?: boolean): GridFilterOperator<any
},
{
value: 'onOrBefore',
getApplyFilterFn: (filterItem) => {
getApplyFilterFnV7: (filterItem) => {
return buildApplyFilterFn(filterItem, (value1, value2) => value1 <= value2, showTime);
},
InputComponent: GridFilterInputDate,
InputComponentProps: { type: showTime ? 'datetime-local' : 'date' },
},
{
value: 'isEmpty',
getApplyFilterFn: () => {
return ({ value }): boolean => {
getApplyFilterFnV7: () => {
return (value, _, __, ___): boolean => {
return value == null;
};
},
requiresFilterValue: false,
},
{
value: 'isNotEmpty',
getApplyFilterFn: () => {
return ({ value }): boolean => {
getApplyFilterFnV7: () => {
return (value, _, __, ___): boolean => {
return value != null;
};
},
Expand Down
2 changes: 1 addition & 1 deletion packages/grid/x-data-grid/src/colDef/gridNumericColDef.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,5 @@ export const GRID_NUMERIC_COL_DEF: GridColTypeDef<number | string | null, string
valueParser: (value) => (value === '' ? null : Number(value)),
valueFormatter: ({ value }) => (isNumber(value) ? value.toLocaleString() : value || ''),
filterOperators: getGridNumericOperators(),
getApplyQuickFilterFn: getGridNumericQuickFilterFn,
getApplyQuickFilterFnV7: getGridNumericQuickFilterFn,
};
Loading