From 5db2ef12756062c274134d69a193155f7f91873d Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Thu, 15 Apr 2021 07:46:28 -0700 Subject: [PATCH] Perf fixes Summary: This diff fixes some more perf bottlenecks in the layout inspector (see the diffs earlier in the stack for the total picture). Mostly: 1. Pass down stable refs from the root for callbacks and configuration 2. Remove the deep-equality check in the sidebar section rendering, which has a pretty significant constant overhead, especially if the selection didn't change 3. If the selection changes, the correct semantics is to reset the sidebar rather than trying to reconcile the elements. (A consequence of this is that Panel collapse state isn't preserved atm after changing selection, will address that in a later diff) This reduces average render time for sidebar from ~20 to ~2 ms. Reviewed By: priteshrnandgaonkar Differential Revision: D27677353 fbshipit-source-id: ba183b7e3d778c0b3c8e7ca0d51535ce99a097ca --- desktop/flipper-plugin/src/ui/Sidebar.tsx | 1 + .../src/ui/data-inspector/DataInspector.tsx | 4 +- .../src/ui/elements-inspector/elements.tsx | 134 +++---- .../public/layout/InspectorSidebar.tsx | 17 +- desktop/plugins/public/layout/index.tsx | 1 + desktop/plugins/public/layout/package.json | 1 - desktop/plugins/public/yarn.lock | 353 ------------------ 7 files changed, 80 insertions(+), 431 deletions(-) diff --git a/desktop/flipper-plugin/src/ui/Sidebar.tsx b/desktop/flipper-plugin/src/ui/Sidebar.tsx index c7c4e0747..4bbab3d1a 100644 --- a/desktop/flipper-plugin/src/ui/Sidebar.tsx +++ b/desktop/flipper-plugin/src/ui/Sidebar.tsx @@ -145,6 +145,7 @@ export class Sidebar extends Component { let minWidth: number | undefined; let maxWidth: number | undefined; + // TODO: memo const resizable: {[key: string]: boolean} = {}; if (position === 'left') { resizable.right = true; diff --git a/desktop/flipper-plugin/src/ui/data-inspector/DataInspector.tsx b/desktop/flipper-plugin/src/ui/data-inspector/DataInspector.tsx index c1acef362..662dfc1ec 100644 --- a/desktop/flipper-plugin/src/ui/data-inspector/DataInspector.tsx +++ b/desktop/flipper-plugin/src/ui/data-inspector/DataInspector.tsx @@ -31,6 +31,8 @@ export {DataValueExtractor} from './DataPreview'; export const RootDataContext = createContext<() => any>(() => ({})); +const contextMenuTrigger = ['contextMenu' as const]; + const BaseContainer = styled.div<{depth?: number; disabled?: boolean}>( (props) => ({ fontFamily: 'Menlo, monospace', @@ -622,7 +624,7 @@ const DataInspector: React.FC = memo( - + {expandedPaths && {expandGlyph}} {descriptionOrPreview} diff --git a/desktop/flipper-plugin/src/ui/elements-inspector/elements.tsx b/desktop/flipper-plugin/src/ui/elements-inspector/elements.tsx index 1b705621d..3b1373dfe 100644 --- a/desktop/flipper-plugin/src/ui/elements-inspector/elements.tsx +++ b/desktop/flipper-plugin/src/ui/elements-inspector/elements.tsx @@ -18,6 +18,7 @@ import {tryGetFlipperLibImplementation} from 'flipper-plugin/src/plugin/FlipperL import {DownOutlined, RightOutlined} from '@ant-design/icons'; const {Text} = Typography; +const contextMenuTrigger = ['contextMenu' as const]; export const ElementsConstants = { rowHeight: 23, @@ -229,7 +230,7 @@ type ElementsRowProps = { style?: Object; contextMenuExtensions?: () => Array; decorateRow?: DecorateRow; - forwardedRef: React.Ref | null; + forwardedRef?: React.Ref | null; }; type ElementsRowState = { @@ -377,18 +378,7 @@ class ElementsRow extends PureComponent { const decoration = decorateRow ? decorateRow(element) - : (() => { - switch (element.decoration) { - case 'litho': - return ; - case 'componentkit': - return ; - case 'accessibility': - return ; - default: - return null; - } - })(); + : defaultDecorateRow(element); // when we hover over or select an expanded element with children, we show a line from the // bottom of the element to the next sibling @@ -403,7 +393,7 @@ class ElementsRow extends PureComponent { + trigger={contextMenuTrigger}> { } } +function defaultDecorateRow(element: Element) { + switch (element.decoration) { + case 'litho': + return ; + case 'componentkit': + return ; + case 'accessibility': + return ; + default: + return null; + } +} + function containsKeyInSearchResults( searchResults: ElementSearchResultSet | undefined | null, key: ElementID, @@ -657,17 +660,66 @@ export class Elements extends PureComponent { } }; + onElementHoveredHandler = (key: string | null | undefined) => { + this.props.onElementHovered?.(key); + }; + + onCopyExpandedTree = ( + element: Element, + maxDepth: number, + depth: number = 0, + ): string => { + const shouldIncludeChildren = element.expanded && depth < maxDepth; + const children = shouldIncludeChildren + ? element.children.map((childId) => { + const childElement = this.props.elements[childId]; + return childElement == null + ? '' + : this.onCopyExpandedTree(childElement, maxDepth, depth + 1); + }) + : []; + + const childrenValue = children.toString().replace(',', ''); + const indentation = depth === 0 ? '' : '\n'.padEnd(depth * 2 + 1, ' '); + const attrs = element.attributes.reduce( + (acc, val) => acc + ` ${val.name}=${val.value}`, + '', + ); + + return `${indentation}${element.name}${attrs}${childrenValue}`; + }; + + scrollToSelectionRefHandler = (selectedRow: HTMLDivElement | null) => { + if ( + !selectedRow || + !this._outerRef.current || + this.props.selected === this.state.scrolledElement + ) { + return; + } + this.setState({scrolledElement: this.props.selected}); + const outer = this._outerRef.current; + if (outer.scrollTo) { + outer.scrollTo({ + top: this._calculateScrollTop( + outer.offsetHeight, + outer.scrollTop, + selectedRow.offsetHeight, + selectedRow.offsetTop, + ), + }); + } + }; + buildRow = (row: FlatElement, index: number) => { const { onElementExpanded, - onElementHovered, onElementSelected, selected, focused, searchResults, contextMenuExtensions, decorateRow, - elements, } = this.props; const {flatElements} = this.state; @@ -685,30 +737,6 @@ export class Elements extends PureComponent { if (this.props.alternateRowColor) { isEven = index % 2 === 0; } - const onCopyExpandedTree = ( - maxDepth: number, - element: Element, - depth: number, - ): string => { - const shouldIncludeChildren = element.expanded && depth < maxDepth; - const children = shouldIncludeChildren - ? element.children.map((childId) => { - const childElement = elements[childId]; - return childElement == null - ? '' - : onCopyExpandedTree(maxDepth, childElement, depth + 1); - }) - : []; - - const childrenValue = children.toString().replace(',', ''); - const indentation = depth === 0 ? '' : '\n'.padEnd(depth * 2 + 1, ' '); - const attrs = element.attributes.reduce( - (acc, val) => acc + ` ${val.name}=${val.value}`, - '', - ); - - return `${indentation}${element.name}${attrs}${childrenValue}`; - }; return ( { key={row.key} even={isEven} onElementExpanded={onElementExpanded} - onElementHovered={(key: string | null | undefined) => { - onElementHovered && onElementHovered(key); - }} + onElementHovered={this.onElementHoveredHandler} onElementSelected={onElementSelected} - onCopyExpandedTree={(element, maxDepth) => - onCopyExpandedTree(maxDepth, element, 0) - } + onCopyExpandedTree={this.onCopyExpandedTree} selected={selected === row.key} focused={focused === row.key} matchingSearchQuery={ @@ -738,23 +762,7 @@ export class Elements extends PureComponent { decorateRow={decorateRow} forwardedRef={ selected == row.key && this.state.scrolledElement !== selected - ? (selectedRow) => { - if (!selectedRow || !this._outerRef.current) { - return; - } - this.setState({scrolledElement: selected}); - const outer = this._outerRef.current; - if (outer.scrollTo) { - outer.scrollTo({ - top: this._calculateScrollTop( - outer.offsetHeight, - outer.scrollTop, - selectedRow.offsetHeight, - selectedRow.offsetTop, - ), - }); - } - } + ? this.scrollToSelectionRefHandler : null } /> diff --git a/desktop/plugins/public/layout/InspectorSidebar.tsx b/desktop/plugins/public/layout/InspectorSidebar.tsx index a3a2ed1bb..b9517da26 100644 --- a/desktop/plugins/public/layout/InspectorSidebar.tsx +++ b/desktop/plugins/public/layout/InspectorSidebar.tsx @@ -18,8 +18,7 @@ import { Client, Logger, } from 'flipper'; -import {Component} from 'react'; -import deepEqual from 'deep-equal'; +import {PureComponent} from 'react'; import React from 'react'; import {useMemo, useEffect} from 'react'; import {kebabCase} from 'lodash'; @@ -39,21 +38,13 @@ type InspectorSidebarSectionProps = { tooltips?: Object; }; -class InspectorSidebarSection extends Component { +class InspectorSidebarSection extends PureComponent { setValue = (path: Array, value: any) => { if (this.props.onValueChanged) { this.props.onValueChanged([this.props.id, ...path], value); } }; - shouldComponentUpdate(nextProps: InspectorSidebarSectionProps) { - return ( - !deepEqual(nextProps, this.props) || - this.props.id !== nextProps.id || - this.props.onValueChanged !== nextProps.onValueChanged - ); - } - extractValue = (val: any, _depth: number) => { if (val && val.__type__) { return { @@ -142,7 +133,7 @@ const Sidebar: React.FC = (props: Props) => { } return [sectionDefs, sectionKeys]; - }, [props.element]); + }, [element]); const sections: Array = ( (SidebarExtensions && @@ -173,7 +164,7 @@ const Sidebar: React.FC = (props: Props) => { sectionKeys.map((key) => props.logger.track('usage', `layout-sidebar-extension:${key}:loaded`), ); - }, [props.element?.data]); + }, [sectionKeys.join(',')]); if (!element || !element.data) { return No data; diff --git a/desktop/plugins/public/layout/index.tsx b/desktop/plugins/public/layout/index.tsx index 7636141de..292808bd9 100644 --- a/desktop/plugins/public/layout/index.tsx +++ b/desktop/plugins/public/layout/index.tsx @@ -521,6 +521,7 @@ export default class LayoutPlugin extends FlipperPlugin<