From 2dad8809c4358e2ba3c48bbda9f1e3345be78d10 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Thu, 20 Feb 2020 04:37:53 -0800 Subject: [PATCH] Fix performance issues [1/N] Summary: This issue fixes a bunch of performance issues: * The introduction of a context menu around every _row_ in D18808544 breaks the internal contract between lists and rows that react-window has, causing empty element to appear during scrolling, and some optimizations not working. Fixed by wrapping the context menu at the right level * Every time a new row is created for the listview, it gets fresh event handlers and column configurations. THis has been fixed by precomputing the column configuration and avoiding the need to close over the row data in the event handlers * Added some stricter immutable typings to make sure we don't break the immutable contract somewhere * Fix the introduction of on the fly styling generation, which isn't needed Reviewed By: passy Differential Revision: D19853595 fbshipit-source-id: dc82b6586889f4e8c7a437cfdc27a50dc33ba2a2 --- src/devices/BaseDevice.tsx | 14 ++-- src/plugins/logs/LogWatcher.tsx | 16 ++-- src/plugins/logs/index.tsx | 31 ++++---- src/ui/components/table/ManagedTable.tsx | 93 +++++++++++++----------- src/ui/components/table/TableRow.tsx | 22 ++++-- 5 files changed, 100 insertions(+), 76 deletions(-) diff --git a/src/devices/BaseDevice.tsx b/src/devices/BaseDevice.tsx index a7161cbb1..caec4f628 100644 --- a/src/devices/BaseDevice.tsx +++ b/src/devices/BaseDevice.tsx @@ -21,13 +21,13 @@ export type LogLevel = | 'fatal'; export type DeviceLogEntry = { - date: Date; - pid: number; - tid: number; - app?: string; - type: LogLevel; - tag: string; - message: string; + readonly date: Date; + readonly pid: number; + readonly tid: number; + readonly app?: string; + readonly type: LogLevel; + readonly tag: string; + readonly message: string; }; export type DeviceShell = { diff --git a/src/plugins/logs/LogWatcher.tsx b/src/plugins/logs/LogWatcher.tsx index 41da1b8bd..841917e73 100644 --- a/src/plugins/logs/LogWatcher.tsx +++ b/src/plugins/logs/LogWatcher.tsx @@ -24,20 +24,20 @@ import { import React from 'react'; export type Counter = { - expression: RegExp; - count: number; - notify: boolean; - label: string; + readonly expression: RegExp; + readonly count: number; + readonly notify: boolean; + readonly label: string; }; type Props = { - onChange: (counters: Array) => void; - counters: Array; + readonly onChange: (counters: ReadonlyArray) => void; + readonly counters: ReadonlyArray; }; type State = { - input: string; - highlightedRow: string | null; + readonly input: string; + readonly highlightedRow: string | null; }; const ColumnSizes = { diff --git a/src/plugins/logs/index.tsx b/src/plugins/logs/index.tsx index 096c58d27..1eb3965b1 100644 --- a/src/plugins/logs/index.tsx +++ b/src/plugins/logs/index.tsx @@ -40,20 +40,20 @@ import {MenuTemplate} from 'src/ui/components/ContextMenu'; const LOG_WATCHER_LOCAL_STORAGE_KEY = 'LOG_WATCHER_LOCAL_STORAGE_KEY'; -type Entries = Array<{ - row: TableBodyRow; - entry: DeviceLogEntry; +type Entries = ReadonlyArray<{ + readonly row: TableBodyRow; + readonly entry: DeviceLogEntry; }>; type BaseState = { - rows: Array; - entries: Entries; - key2entry: {[key: string]: DeviceLogEntry}; + readonly rows: ReadonlyArray; + readonly entries: Entries; + readonly key2entry: {readonly [key: string]: DeviceLogEntry}; }; type AdditionalState = { - highlightedRows: Set; - counters: Array; + readonly highlightedRows: ReadonlySet; + readonly counters: ReadonlyArray; }; type State = BaseState & AdditionalState; @@ -95,7 +95,7 @@ const COLUMN_SIZE = { tag: 120, app: 200, message: 'flex', -}; +} as const; const COLUMNS = { type: { @@ -119,7 +119,7 @@ const COLUMNS = { message: { value: 'Message', }, -}; +} as const; const INITIAL_COLUMN_ORDER = [ { @@ -150,7 +150,7 @@ const INITIAL_COLUMN_ORDER = [ key: 'message', visible: true, }, -]; +] as const; const LOG_TYPES: { [level: string]: { @@ -260,7 +260,7 @@ export function addEntriesToState( rows: [], entries: [], key2entry: {}, - }, + } as const, ): BaseState { const rows = [...state.rows]; const entries = [...state.entries]; @@ -421,7 +421,7 @@ export default class LogTable extends FlipperDevicePlugin< calculateHighlightedRows = ( deepLinkPayload: string | null, - rows: Array, + rows: ReadonlyArray, ): Set => { const highlightedRows = new Set(); if (!deepLinkPayload) { @@ -459,7 +459,10 @@ export default class LogTable extends FlipperDevicePlugin< columnOrder: TableColumnOrder; logListener: Symbol | undefined; - batch: Entries = []; + batch: Array<{ + readonly row: TableBodyRow; + readonly entry: DeviceLogEntry; + }> = []; queued: boolean = false; counter: number = 0; diff --git a/src/ui/components/table/ManagedTable.tsx b/src/ui/components/table/ManagedTable.tsx index f8b56f781..64b1b1992 100644 --- a/src/ui/components/table/ManagedTable.tsx +++ b/src/ui/components/table/ManagedTable.tsx @@ -34,6 +34,9 @@ import {DEFAULT_ROW_HEIGHT} from './types'; import textContent from '../../../utils/textContent'; import {notNull} from '../../../utils/typeUtils'; +const EMPTY_OBJECT = {}; +Object.freeze(EMPTY_OBJECT); + export type ManagedTableProps = { /** * Column definitions. @@ -140,6 +143,7 @@ type ManagedTableState = { highlightedRows: Set; sortOrder?: TableRowSortOrder; columnOrder: TableColumnOrder; + columnKeys: string[]; columnSizes: TableColumnSizes; shouldScrollToBottom: boolean; }; @@ -172,20 +176,6 @@ export class ManagedTable extends React.Component< ); }; - state: ManagedTableState = { - columnOrder: - JSON.parse(window.localStorage.getItem(this.getTableKey()) || 'null') || - this.props.columnOrder || - Object.keys(this.props.columns).map(key => ({key, visible: true})), - columnSizes: - this.props.tableKey && globalTableState[this.props.tableKey] - ? globalTableState[this.props.tableKey] - : this.props.columnSizes || {}, - highlightedRows: this.props.highlightedRows || new Set(), - sortOrder: this.props.initialSortOrder || undefined, - shouldScrollToBottom: Boolean(this.props.stickyBottom), - }; - tableRef = React.createRef(); scrollRef: { @@ -200,6 +190,25 @@ export class ManagedTable extends React.Component< // trigger actions on the first update instead. firstUpdate = true; + constructor(props: ManagedTableProps) { + super(props); + const columnOrder = + JSON.parse(window.localStorage.getItem(this.getTableKey()) || 'null') || + this.props.columnOrder || + Object.keys(this.props.columns).map(key => ({key, visible: true})); + this.state = { + columnOrder, + columnKeys: this.computeColumnKeys(columnOrder), + columnSizes: + this.props.tableKey && globalTableState[this.props.tableKey] + ? globalTableState[this.props.tableKey] + : this.props.columnSizes || {}, + highlightedRows: this.props.highlightedRows || new Set(), + sortOrder: this.props.initialSortOrder || undefined, + shouldScrollToBottom: Boolean(this.props.stickyBottom), + }; + } + componentDidMount() { document.addEventListener('keydown', this.onKeyDown); } @@ -233,6 +242,7 @@ export class ManagedTable extends React.Component< } this.setState({ columnOrder: nextProps.columnOrder, + columnKeys: this.computeColumnKeys(nextProps.columnOrder), }); } @@ -277,6 +287,10 @@ export class ManagedTable extends React.Component< this.firstUpdate = false; } + computeColumnKeys(columnOrder: TableColumnOrder) { + return columnOrder.map(k => (k.visible ? k.key : null)).filter(notNull); + } + scrollToHighlightedRows = () => { const {current} = this.tableRef; const {highlightedRows} = this.state; @@ -609,34 +623,24 @@ export class ManagedTable extends React.Component< getRow = ({index, style}: {index: number; style: React.CSSProperties}) => { const {onAddFilter, multiline, zebra, rows} = this.props; - const {columnOrder, columnSizes, highlightedRows} = this.state; - const columnKeys: Array = columnOrder - .map(k => (k.visible ? k.key : null)) - .filter(notNull); + const {columnKeys, columnSizes, highlightedRows} = this.state; return ( - - this.onHighlight(e, rows[index], index)} - onMouseEnter={e => this.onMouseEnterRow(e, rows[index], index)} - multiline={multiline} - rowLineHeight={24} - highlighted={highlightedRows.has(rows[index].key)} - row={rows[index]} - index={index} - style={ - rows[index].height ? {...style, height: rows[index].height} : style - } - onAddFilter={onAddFilter} - zebra={zebra} - /> - + ); }; @@ -686,7 +690,14 @@ export class ManagedTable extends React.Component< )} {this.props.autoHeight ? ( - this.props.rows.map((_, index) => this.getRow({index, style: {}})) + + {this.props.rows.map((_, index) => + this.getRow({index, style: EMPTY_OBJECT}), + )} + ) : ( {({width, height}) => ( diff --git a/src/ui/components/table/TableRow.tsx b/src/ui/components/table/TableRow.tsx index 9f5474cfa..0cf5c8d68 100644 --- a/src/ui/components/table/TableRow.tsx +++ b/src/ui/components/table/TableRow.tsx @@ -115,8 +115,12 @@ TableBodyColumnContainer.displayName = 'TableRow:TableBodyColumnContainer'; type Props = { columnSizes: TableColumnSizes; columnKeys: TableColumnKeys; - onMouseDown: (e: React.MouseEvent) => any; - onMouseEnter?: (e: React.MouseEvent) => void; + onMouseDown: (e: React.MouseEvent, row: TableBodyRow, index: number) => void; + onMouseEnter?: ( + e: React.MouseEvent, + row: TableBodyRow, + index: number, + ) => void; multiline?: boolean; rowLineHeight: number; highlighted: boolean; @@ -132,6 +136,14 @@ export default class TableRow extends React.PureComponent { zebra: true, }; + handleMouseDown = (e: React.MouseEvent) => { + this.props.onMouseDown(e, this.props.row, this.props.index); + }; + + handleMouseEnter = (e: React.MouseEvent) => { + this.props.onMouseEnter?.(e, this.props.row, this.props.index); + }; + render() { const { index, @@ -142,8 +154,6 @@ export default class TableRow extends React.PureComponent { multiline, columnKeys, columnSizes, - onMouseEnter, - onMouseDown, zebra, onAddFilter, } = this.props; @@ -157,8 +167,8 @@ export default class TableRow extends React.PureComponent { multiline={multiline} even={index % 2 === 0} zebra={zebra} - onMouseDown={onMouseDown} - onMouseEnter={onMouseEnter} + onMouseDown={this.handleMouseDown} + onMouseEnter={this.handleMouseEnter} style={style} highlightOnHover={row.highlightOnHover} data-key={row.key}