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

[table] Perf improvements with selections #2005

Merged
merged 13 commits into from
Nov 2, 2018
Merged

[table] Perf improvements with selections #2005

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
2a04c30
table performance improvements - reducing rerendering where possible
Jan 16, 2018
7d3780e
reinstate enum strings
Jan 16, 2018
e6ba864
fix compile errors
Jan 16, 2018
ef2688e
fix lint errors
Jan 16, 2018
61d93d3
fix test
Jan 16, 2018
23ecc46
cell: revert changes
Jan 17, 2018
6173d33
Merge remote-tracking branch 'upstream/develop' into feature/table-pe…
Jan 24, 2018
00097ed
Merge branch 'develop' into feature/table-perf-improvements
Feb 9, 2018
e99ee77
code review suggestions; add forceRerenderOnSelectionChange flag
Feb 9, 2018
f660043
Merge remote-tracking branch 'upstream/develop' into feature/table-pe…
Mar 22, 2018
5873014
Merge branch 'develop' into feature/table-perf-improvements
Oct 29, 2018
1e1bae2
lint
Oct 29, 2018
8d85259
default to false
giladgray Nov 1, 2018
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
4 changes: 4 additions & 0 deletions packages/table-dev-app/src/mutableTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ export interface IMutableTableState {
scrollToRegionType?: RegionCardinality;
scrollToRowIndex?: number;
selectedFocusStyle?: FocusStyle;
selectedRegions: IRegion[];
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be optional. i think that's the source of compiler errors.

but also you have lint failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

showCallbackLogs?: boolean;
showCellsLoading?: boolean;
showColumnHeadersLoading?: boolean;
Expand Down Expand Up @@ -256,6 +257,7 @@ const DEFAULT_STATE: IMutableTableState = {
scrollToRegionType: RegionCardinality.CELLS,
scrollToRowIndex: 0,
selectedFocusStyle: FocusStyle.TAB,
selectedRegions: [],
showCallbackLogs: true,
showCellsLoading: false,
showColumnHeadersLoading: false,
Expand Down Expand Up @@ -375,6 +377,7 @@ export class MutableTable extends React.Component<{}, IMutableTableState> {
renderMode={this.state.renderMode}
rowHeaderCellRenderer={this.renderRowHeader}
selectionModes={this.getEnabledSelectionModes()}
selectedRegions={this.state.selectedRegions}
styledRegionGroups={this.getStyledRegionGroups()}
>
{this.renderColumns()}
Expand Down Expand Up @@ -905,6 +908,7 @@ export class MutableTable extends React.Component<{}, IMutableTableState> {

private onSelection = (selectedRegions: IRegion[]) => {
this.maybeLogCallback(`[onSelection] selectedRegions =`, ...selectedRegions);
this.setState({selectedRegions});
};

private onColumnsReordered = (oldIndex: number, newIndex: number, length: number) => {
Expand Down
17 changes: 8 additions & 9 deletions packages/table/src/cell/cell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import * as classNames from "classnames";
import * as React from "react";
import * as Classes from "../common/classes";

import { Classes as CoreClasses, IIntentProps, IProps, Utils as CoreUtils } from "@blueprintjs/core";
import { Classes as CoreClasses, IIntentProps, IProps } from "@blueprintjs/core";

import { LoadableContent } from "../common/loadableContent";
import { JSONFormat } from "./formats/jsonFormat";
Expand Down Expand Up @@ -93,20 +93,19 @@ export type ICellRenderer = (rowIndex: number, columnIndex: number) => React.Rea

export const emptyCellRenderer = () => <Cell />;

/*
* Not a PureComponent because any children don't get included in the shallow comparison.
* - if we have children we always want to rerender in case the children have changed.
* - if we don't have children the rerender is so cheap it isn't a problem.
*
* Note that due to React's typings we can't check for the presence of children in shouldComponentUpdate() in a typesafe way.
*/
export class Cell extends React.Component<ICellProps, {}> {
public static defaultProps = {
truncated: true,
wrapText: false,
};

public shouldComponentUpdate(nextProps: ICellProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a concerning deletion. input from @themadcreator @cmslewis on the impact here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I suppose it is inconsistent with my comment above where I claim that children can be memoized, so the shallow compare would be useful. Might be better to revert to the old code and add a comment making clear that the shallow compare will fail on children unless they are memoized.

// deeply compare "style," because a new but identical object might have been provided.
return (
!CoreUtils.shallowCompareKeys(this.props, nextProps, { exclude: ["style"] }) ||
!CoreUtils.deepCompareKeys(this.props.style, nextProps.style)
);
}

public render() {
const {
cellRef,
Expand Down
4 changes: 2 additions & 2 deletions packages/table/src/common/batcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,14 +129,14 @@ export class Batcher<T> {
const keysToAdd = this.setKeysDifference(this.batchArgs, this.currentObjects, addNewLimit);
keysToAdd.forEach(key => (this.currentObjects[key] = callback.apply(undefined, this.batchArgs[key])));

// set `done` to true of sets match exactly after add/remove and there
// set `done` to true if sets match exactly after add/remove and there
// are no "old objects" remaining
this.done =
this.setHasSameKeys(this.batchArgs, this.currentObjects) && Object.keys(this.oldObjects).length === 0;
}

/**
* Returns true of the "current" set matches the "batch" set.
* Returns true if the "current" set matches the "batch" set.
*/
public isDone() {
return this.done;
Expand Down
68 changes: 48 additions & 20 deletions packages/table/src/table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,28 +681,36 @@ export class Table extends AbstractComponent<ITableProps, ITableState> {
selectionModes,
} = nextProps;

const newChildArray = React.Children.toArray(children) as Array<React.ReactElement<IColumnProps>>;
const hasNewChildren = this.props.children !== nextProps.children;
Copy link
Contributor

Choose a reason for hiding this comment

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

if i'm not mistaken, this will be true every time as children is a new object reference every time (never ===)?
perhaps add a console.log(hasNewChildren) locally to debug if it's ever false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

children may be memoized

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: hasNewChildren sort of implies to me that there are additional children. maybe call this didChildrenChange.

Also, RE: "may be": do you mean the children will be memoized only if a developer does so explicitly?

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 exactly - it needs to be explicitly done by the developer. Memoizing props is a standard part of getting better perf in react so seems reasonable?

const newChildArray = hasNewChildren ? React.Children.toArray(children) as Array<React.ReactElement<IColumnProps>> : this.childrenArray;
const numCols = newChildArray.length;

// Try to maintain widths of columns by looking up the width of the
// column that had the same `ID` prop. If none is found, use the
// previous width at the same index.
const previousColumnWidths = newChildArray.map((child: React.ReactElement<IColumnProps>, index: number) => {
const mappedIndex = this.columnIdToIndex[child.props.id];
return this.state.columnWidths[mappedIndex != null ? mappedIndex : index];
});

// Make sure the width/height arrays have the correct length, but keep
// as many existing widths/heights when possible. Also, apply the
// sparse width/heights from props.
let gridInvalidationRequired = false;
giladgray marked this conversation as resolved.
Show resolved Hide resolved
let newColumnWidths = this.state.columnWidths;
newColumnWidths = Utils.arrayOfLength(newColumnWidths, numCols, defaultColumnWidth);
newColumnWidths = Utils.assignSparseValues(newColumnWidths, previousColumnWidths);
newColumnWidths = Utils.assignSparseValues(newColumnWidths, columnWidths);
if (defaultColumnWidth !== this.props.defaultColumnWidth || columnWidths !== this.props.columnWidths || (hasNewChildren && !this.areChildrenSameShape(newChildArray))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

columnWidths !== this.props.columnWidths is problematic as array is likely to be a new instance?

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 guess my assumption here is that clients who are performance-sensitive will take care to memoize these arrays where possible, which is standard practice to get the benefits of pure-render semantics in react.

// Try to maintain widths of columns by looking up the width of the
// column that had the same `ID` prop. If none is found, use the
// previous width at the same index.
const previousColumnWidths = newChildArray.map((child: React.ReactElement<IColumnProps>, index: number) => {
const mappedIndex = this.columnIdToIndex[child.props.id];
return this.state.columnWidths[mappedIndex != null ? mappedIndex : index];
});

// Make sure the width/height arrays have the correct length, but keep
// as many existing widths/heights when possible. Also, apply the
giladgray marked this conversation as resolved.
Show resolved Hide resolved
// sparse width/heights from props.
newColumnWidths = Utils.arrayOfLength(newColumnWidths, numCols, defaultColumnWidth);
newColumnWidths = Utils.assignSparseValues(newColumnWidths, previousColumnWidths);
newColumnWidths = Utils.assignSparseValues(newColumnWidths, columnWidths);
gridInvalidationRequired = true;
giladgray marked this conversation as resolved.
Show resolved Hide resolved
}

let newRowHeights = this.state.rowHeights;
newRowHeights = Utils.arrayOfLength(newRowHeights, numRows, defaultRowHeight);
newRowHeights = Utils.assignSparseValues(newRowHeights, rowHeights);
if (defaultRowHeight !== this.props.defaultRowHeight || rowHeights !== this.props.rowHeights) {
newRowHeights = Utils.arrayOfLength(newRowHeights, numRows, defaultRowHeight);
newRowHeights = Utils.assignSparseValues(newRowHeights, rowHeights);
gridInvalidationRequired = true;
}

let newSelectedRegions = selectedRegions;
if (selectedRegions == null) {
Expand All @@ -724,9 +732,13 @@ export class Table extends AbstractComponent<ITableProps, ITableState> {
newSelectedRegions,
);

this.childrenArray = newChildArray;
this.columnIdToIndex = Table.createColumnIdIndex(this.childrenArray);
this.invalidateGrid();
if (hasNewChildren) {
this.childrenArray = newChildArray;
this.columnIdToIndex = Table.createColumnIdIndex(this.childrenArray);
}
if (gridInvalidationRequired) {
this.invalidateGrid();
}
this.setState({
columnWidths: newColumnWidths,
focusedCell: newFocusedCell,
Expand All @@ -737,6 +749,22 @@ export class Table extends AbstractComponent<ITableProps, ITableState> {
});
}

private areChildrenSameShape(newChildArray: Array<React.ReactElement<IColumnProps>>) {
if (this.childrenArray.length !== newChildArray.length) {
return false;
}
return this.childrenArray.every((child, index) => {
const newChild = newChildArray[index];
if (child === newChild) {
return true;
}
if (child == null || newChild == null) {
return false;
}
return child.props.id === newChild.props.id;
});
}

public render() {
const {
children,
Expand Down