Performance optimizations

Summary:
This diff removes a bunch of performance bottlenecks of `DataInspector`, mostly by making sure that non new data structures are send to children during rendering. For example, before this diff, fields like `expanded`, `ancestry` and `path` would always be freshly constructed, resulting in new data structures send down, causing the full json tree to always re-render.

By migrating to hooks this became a lot easy to manage.

Also fixed some other minor component reuse issues

Fixed rendering of recursive trees which was broken in the past, and added regression test

Fixed issue with uppercase search string causing unnecessary re-filtering

Make sure changing expand / collapse resets the filter

Reviewed By: passy

Differential Revision: D21381647

fbshipit-source-id: 72834e15088432f55b4b9c88f182ffc9908d4e49

desktop/app/src/ui/components/data-inspector/DataDescription.tsx

@@ -12,7 +12,6 @@ import {DataInspectorSetValue} from './DataInspector';
 import {PureComponent} from 'react';
 import styled from '@emotion/styled';
 import {SketchPicker, CompactPicker} from 'react-color';
-import {Component, Fragment} from 'react';
 import Popover from '../Popover';
 import {colors} from '../colors';
 import Input from '../Input';
@@ -276,7 +275,7 @@ export default class DataDescription extends PureComponent<
           type={this.props.type}
           value={this.props.value}
           extra={this.props.extra}
-          editable={Boolean(this.props.setValue)}
+          editable={!!this.props.setValue}
           commit={this.commit}
           onEdit={this.onEditStart}
         />
@@ -359,7 +358,7 @@ class ColorEditor extends PureComponent<{
   render() {
     const colorInfo = parseColor(this.props.value);
     if (!colorInfo) {
-      return <Fragment />;
+      return null;
     }
 
     return (
@@ -440,7 +439,7 @@ class ColorEditor extends PureComponent<{
   }
 }
 
-class DataDescriptionPreview extends Component<{
+class DataDescriptionPreview extends PureComponent<{
   type: string;
   value: any;
   extra?: any;
@@ -539,7 +538,9 @@ function parseColor(
   return {a, b, g, r};
 }
 
-class DataDescriptionContainer extends Component<{
+const pencilStyle = {cursor: 'pointer', marginLeft: 8};
+
+class DataDescriptionContainer extends PureComponent<{
   type: string;
   value: any;
   editable: boolean;
@@ -563,7 +564,7 @@ class DataDescriptionContainer extends Component<{
 
     switch (type) {
       case 'number':
-        return <NumberValue>{Number(val)}</NumberValue>;
+        return <NumberValue>{+val}</NumberValue>;
 
       case 'color': {
         const colorInfo = parseColor(val);
@@ -620,7 +621,7 @@ class DataDescriptionContainer extends Component<{
                 variant="outline"
                 color={colors.light20}
                 size={16}
-                style={{cursor: 'pointer', marginLeft: 8}}
+                style={pencilStyle}
               />
             </>
           );
@@ -637,12 +638,12 @@ class DataDescriptionContainer extends Component<{
         return editable ? (
           <input
             type="checkbox"
-            checked={Boolean(val)}
+            checked={!!val}
             disabled={!editable}
             onChange={this.onChangeCheckbox}
           />
         ) : (
-          <StringValue>{String(val)}</StringValue>
+          <StringValue>{'' + val}</StringValue>
         );
 
       case 'undefined':

desktop/app/src/ui/components/data-inspector/DataInspector.tsx

@@ -9,7 +9,7 @@
 
 import DataDescription from './DataDescription';
 import {MenuTemplate} from '../ContextMenu';
-import {Component} from 'react';
+import {memo, useMemo, useRef, useState, useEffect, useCallback} from 'react';
 import ContextMenu from '../ContextMenu';
 import Tooltip from '../Tooltip';
 import styled from '@emotion/styled';
@@ -19,11 +19,9 @@ import DataPreview, {DataValueExtractor, InspectorName} from './DataPreview';
 import {getSortedKeys} from './utils';
 import {colors} from '../colors';
 import {clipboard} from 'electron';
-import deepEqual from 'deep-equal';
 import React from 'react';
 import {TooltipOptions} from '../TooltipProvider';
-import {shallowEqual} from 'react-redux';
+import {useHighlighter} from '../Highlight';
-import {HighlightContext} from '../Highlight';
 
 export {DataValueExtractor} from './DataPreview';
 
@@ -116,7 +114,7 @@ type DataInspectorProps = {
   /**
    * An array containing the current location of the data relative to its root.
    */
-  path: Array<string>;
+  parentPath: Array<string>;
   /**
    * Whether to expand the root by default.
    */
@@ -149,7 +147,7 @@ type DataInspectorProps = {
   /**
    * Ancestry of parent objects, used to avoid recursive objects.
    */
-  ancestry: Array<Object>;
+  parentAncestry: Array<Object>;
   /**
    * Object of properties that will have tooltips
    */
@@ -205,7 +203,12 @@ function getRootContextMenu(
     return cached;
   }
 
-  const stringValue = JSON.stringify(data, null, 2);
+  let stringValue: string;
+  try {
+    stringValue = JSON.stringify(data, null, 2);
+  } catch (e) {
+    stringValue = '<circular structure>';
+  }
   const menu: Array<Electron.MenuItemConstructorOptions> = [
     {
       label: 'Copy entire tree',
@@ -307,273 +310,155 @@ type DataInspectorState = {
   resDiff: any;
 };
 
+const recursiveMarker = <RecursiveBaseWrapper>Recursive</RecursiveBaseWrapper>;
+
 /**
  * An expandable data inspector.
  *
  * This component is fairly low level. It's likely you're looking for
  * [`<ManagedDataInspector>`]().
  */
-export default class DataInspector extends Component<
+const DataInspector: React.FC<DataInspectorProps> = memo(
-  DataInspectorProps,
+  function DataInspectorImpl({
-  DataInspectorState
+    data,
-> {
+    depth,
-  static contextType = HighlightContext; // Replace with useHighlighter
+    diff,
-  context!: React.ContextType<typeof HighlightContext>;
+    expandRoot,
+    parentPath,
+    onExpanded,
+    onDelete,
+    extractValue: extractValueProp,
+    expanded: expandedPaths,
+    name,
+    parentAncestry,
+    collapsed,
+    tooltips,
+    setValue: setValueProp,
+  }) {
+    const highlighter = useHighlighter();
 
-  static defaultProps: {
+    const shouldExpand = useRef(false);
-    expanded: DataInspectorExpanded;
+    const expandHandle = useRef(undefined as any);
-    depth: number;
+    const [renderExpanded, setRenderExpanded] = useState(false);
-    path: Array<string>;
+    const path = useMemo(
-    ancestry: Array<Object>;
+      () => (name === undefined ? parentPath : parentPath.concat([name])),
-  } = {
+      [parentPath, name],
-    expanded: {},
-    depth: 0,
-    path: [],
-    ancestry: [],
-  };
-
-  expandHandle: any = undefined;
-  interaction: (name: string, data?: any) => void;
-
-  constructor(props: DataInspectorProps) {
-    super(props);
-    this.interaction = reportInteraction('DataInspector', props.path.join(':'));
-    this.state = {
-      shouldExpand: false,
-      isExpandable: false,
-      isExpanded: false,
-      res: undefined,
-      resDiff: undefined,
-    };
-  }
-
-  static isExpandable(data: any) {
-    return (
-      typeof data === 'object' && data !== null && Object.keys(data).length > 0
     );
-  }
 
-  shouldComponentUpdate(
+    const extractValue = useCallback(
-    nextProps: DataInspectorProps,
+      (data: any, depth: number) => {
-    nextState: DataInspectorState,
+        let res;
-  ) {
+        if (extractValueProp) {
-    if (!shallowEqual(nextState, this.state)) {
+          res = extractValueProp(data, depth);
-      return true;
-    }
-    const {props} = this;
-
-    // check if any expanded paths effect this subtree
-    if (nextProps.expanded !== props.expanded) {
-      const path = nextProps.path.join('.');
-
-      for (const key in nextProps.expanded) {
-        if (key.startsWith(path) === false) {
-          // this key doesn't effect us
-          continue;
         }
-
+        if (!res) {
-        if (nextProps.expanded[key] !== props.expanded[key]) {
+          res = defaultValueExtractor(data, depth);
-          return true;
         }
-      }
+        return res;
-    }
+      },
-
+      [extractValueProp],
-    // basic equality checks for the rest
-    return (
-      nextProps.data !== props.data ||
-      nextProps.diff !== props.diff ||
-      nextProps.name !== props.name ||
-      nextProps.depth !== props.depth ||
-      !deepEqual(nextProps.path, props.path) ||
-      nextProps.onExpanded !== props.onExpanded ||
-      nextProps.onDelete !== props.onDelete ||
-      nextProps.setValue !== props.setValue ||
-      nextProps.collapsed !== props.collapsed ||
-      nextProps.expandRoot !== props.expandRoot
     );
-  }
 
-  static getDerivedStateFromProps(
+    const res = useMemo(() => extractValue(data, depth), [
-    nextProps: DataInspectorProps,
+      extractValue,
-    currentState: DataInspectorState,
+      data,
-  ): DataInspectorState {
+      depth,
-    const {data, depth, diff, expandRoot, path} = nextProps;
+    ]);
-
+    const resDiff = useMemo(() => extractValue(diff, depth), [
-    const res = DataInspector.extractValue(nextProps, data, depth);
+      extractValue,
-    const resDiff = DataInspector.extractValue(nextProps, diff, depth);
+      data,
+      depth,
+    ]);
+    const ancestry = useMemo(
+      () => (res ? parentAncestry!.concat([res.value]) : []),
+      [parentAncestry, res?.value],
+    );
 
+    let isExpandable = false;
     if (!res) {
-      return {
+      shouldExpand.current = false;
-        shouldExpand: false,
+    } else {
-        isExpanded: false,
+      isExpandable = isValueExpandable(res.value);
-        isExpandable: false,
-        res,
-        resDiff,
-      };
     }
 
-    const isExpandable = DataInspector.isExpandable(res.value);
-    let shouldExpand = false;
     if (isExpandable) {
       if (
         expandRoot === true ||
-        DataInspector.shouldBeExpanded(nextProps, path)
+        shouldBeExpanded(expandedPaths, path, collapsed)
       ) {
-        shouldExpand = true;
+        shouldExpand.current = true;
       } else if (resDiff) {
-        shouldExpand = isComponentExpanded(
+        shouldExpand.current = isComponentExpanded(
-          res.value,
+          res!.value,
           resDiff.type,
           resDiff.value,
         );
       }
     }
 
-    return {
+    useEffect(() => {
-      isExpanded: currentState.isExpanded,
+      if (!shouldExpand.current) {
-      shouldExpand,
+        setRenderExpanded(false);
-      isExpandable,
-      res,
-      resDiff,
-    };
-  }
-
-  static shouldBeExpanded(props: DataInspectorProps, pathParts: Array<string>) {
-    const {expanded} = props;
-
-    // if we have no expanded object then expand everything
-    if (expanded == null) {
-      return true;
-    }
-
-    const path = pathParts.join('.');
-
-    // check if there's a setting for this path
-    if (Object.prototype.hasOwnProperty.call(expanded, path)) {
-      return expanded[path];
-    }
-
-    // check if all paths are collapsed
-    if (props.collapsed === true) {
-      return false;
-    }
-
-    // by default all items are expanded
-    return true;
-  }
-
-  componentDidMount() {
-    this.expandIfNeeded();
-  }
-
-  componentDidUpdate() {
-    this.expandIfNeeded();
-  }
-
-  componentWillUnmount() {
-    cancelIdleCallback(this.expandHandle);
-  }
-
-  expandIfNeeded() {
-    if (this.state.isExpanded !== this.state.shouldExpand) {
-      cancelIdleCallback(this.expandHandle);
-      if (!this.state.shouldExpand) {
-        this.setState({isExpanded: false});
       } else {
-        this.expandHandle = requestIdleCallback(() => {
+        expandHandle.current = requestIdleCallback(() => {
-          this.setState({
+          setRenderExpanded(true);
-            isExpanded: true,
-          });
         });
       }
-    }
+      return () => {
-  }
+        cancelIdleCallback(expandHandle.current);
+      };
+    }, [shouldExpand.current]);
 
-  setExpanded(pathParts: Array<string>, isExpanded: boolean) {
+    const setExpanded = useCallback(
-    const {expanded, onExpanded} = this.props;
+      (pathParts: Array<string>, isExpanded: boolean) => {
-    if (!onExpanded || !expanded) {
+        if (!onExpanded || !expandedPaths) {
-      return;
+          return;
-    }
+        }
-
+        const path = pathParts.join('.');
-    const path = pathParts.join('.');
+        onExpanded(path, isExpanded);
-
+      },
-    onExpanded(path, isExpanded);
+      [onExpanded, expandedPaths],
-  }
-
-  handleClick = () => {
-    cancelIdleCallback(this.expandHandle);
-    const isExpanded = DataInspector.shouldBeExpanded(
-      this.props,
-      this.props.path,
     );
-    this.interaction(isExpanded ? 'collapsed' : 'expanded');
-    this.setExpanded(this.props.path, !isExpanded);
-  };
 
-  handleDelete = (path: Array<string>) => {
+    const handleClick = useCallback(() => {
-    const onDelete = this.props.onDelete;
+      cancelIdleCallback(expandHandle.current);
-    if (!onDelete) {
+      const isExpanded = shouldBeExpanded(expandedPaths, path, collapsed);
-      return;
+      reportInteraction('DataInspector', path.join(':'))(
-    }
+        isExpanded ? 'collapsed' : 'expanded',
+        undefined,
+      );
+      setExpanded(path, !isExpanded);
+    }, [expandedPaths, path, collapsed]);
 
-    onDelete(path);
+    const handleDelete = useCallback(
-  };
+      (path: Array<string>) => {
-
+        if (!onDelete) {
-  static extractValue(props: DataInspectorProps, data: any, depth: number) {
+          return;
-    let res;
+        }
-
+        onDelete(path);
-    const {extractValue} = props;
+      },
-    if (extractValue) {
+      [onDelete],
-      res = extractValue(data, depth);
+    );
-    }
-
-    if (!res) {
-      res = defaultValueExtractor(data, depth);
-    }
-
-    return res;
-  }
-
-  extractValue = (data: any, depth: number) => {
-    return DataInspector.extractValue(this.props, data, depth);
-  };
-
-  render(): any {
-    const {
-      data,
-      diff,
-      depth,
-      expanded: expandedPaths,
-      expandRoot,
-      extractValue,
-      name,
-      onExpanded,
-      onDelete,
-      path,
-      ancestry,
-      collapsed,
-      tooltips,
-    } = this.props;
-
-    const {resDiff, isExpandable, isExpanded, res} = this.state;
-    const highlighter = this.context; // useHighlighter();
 
+    /**
+     * RENDERING
+     */
     if (!res) {
       return null;
     }
 
     // the data inspector makes values read only when setValue isn't set so we just need to set it
     // to null and the readOnly status will be propagated to all children
-    const setValue = res.mutable ? this.props.setValue : null;
+    const setValue = res.mutable ? setValueProp : null;
-    const {type, value, extra} = res;
+    const {value, type, extra} = res;
 
-    if (ancestry.includes(value)) {
+    if (parentAncestry!.includes(value)) {
-      return <RecursiveBaseWrapper>Recursive</RecursiveBaseWrapper>;
+      return recursiveMarker;
     }
 
     let expandGlyph = '';
     if (isExpandable) {
-      if (isExpanded) {
+      if (shouldExpand.current) {
         expandGlyph = '▼';
       } else {
         expandGlyph = '▶';
@@ -585,12 +470,9 @@ export default class DataInspector extends Component<
     }
 
     let propertyNodesContainer = null;
-    if (isExpandable && isExpanded) {
+    if (isExpandable && renderExpanded) {
       const propertyNodes = [];
 
-      // ancestry of children, including its owner object
-      const childAncestry = ancestry.concat([value]);
-
       const diffValue = diff && resDiff ? resDiff.value : null;
 
       const keys = getSortedKeys({...value, ...diffValue});
@@ -600,14 +482,14 @@ export default class DataInspector extends Component<
         for (const metadata of diffMetadataArr) {
           const dataInspectorNode = (
             <DataInspector
-              ancestry={childAncestry}
+              parentAncestry={ancestry}
               extractValue={extractValue}
               setValue={setValue}
               expanded={expandedPaths}
               collapsed={collapsed}
               onExpanded={onExpanded}
               onDelete={onDelete}
-              path={path.concat(key)}
+              parentPath={path}
               depth={depth + 1}
               key={key}
               name={key}
@@ -659,7 +541,7 @@ export default class DataInspector extends Component<
 
     // create description or preview
     let descriptionOrPreview;
-    if (isExpanded || !isExpandable) {
+    if (renderExpanded || !isExpandable) {
       descriptionOrPreview = (
         <DataDescription
           path={path}
@@ -674,7 +556,7 @@ export default class DataInspector extends Component<
         <DataPreview
           type={type}
           value={value}
-          extractValue={this.extractValue}
+          extractValue={extractValue}
           depth={depth}
         />
       );
@@ -689,7 +571,7 @@ export default class DataInspector extends Component<
 
     let wrapperStart;
     let wrapperEnd;
-    if (isExpanded) {
+    if (renderExpanded) {
       if (type === 'object') {
         wrapperStart = <Wrapper>{'{'}</Wrapper>;
         wrapperEnd = <Wrapper>{'}'}</Wrapper>;
@@ -706,8 +588,8 @@ export default class DataInspector extends Component<
     if (isExpandable) {
       contextMenuItems.push(
         {
-          label: isExpanded ? 'Collapse' : 'Expand',
+          label: shouldExpand.current ? 'Collapse' : 'Expand',
-          click: this.handleClick,
+          click: handleClick,
         },
         {
           type: 'separator',
@@ -730,19 +612,16 @@ export default class DataInspector extends Component<
     if (!isExpandable && onDelete) {
       contextMenuItems.push({
         label: 'Delete',
-        click: () => this.handleDelete(this.props.path),
+        click: () => handleDelete(path),
       });
     }
 
     return (
       <BaseContainer
         depth={depth}
-        disabled={
+        disabled={!!setValueProp && !!setValue === false}>
-          Boolean(this.props.setValue) === true && Boolean(setValue) === false
-        }>
         <ContextMenu component="span" items={contextMenuItems}>
-          <PropertyContainer
+          <PropertyContainer onClick={isExpandable ? handleClick : undefined}>
-            onClick={isExpandable ? this.handleClick : undefined}>
             {expandedPaths && <ExpandControl>{expandGlyph}</ExpandControl>}
             {descriptionOrPreview}
             {wrapperStart}
@@ -752,5 +631,88 @@ export default class DataInspector extends Component<
         {wrapperEnd}
       </BaseContainer>
     );
+  },
+  dataInspectorPropsAreEqual,
+);
+
+function shouldBeExpanded(
+  expanded: DataInspectorExpanded,
+  pathParts: Array<string>,
+  collapsed?: boolean,
+) {
+  // if we have no expanded object then expand everything
+  if (expanded == null) {
+    return true;
   }
+
+  const path = pathParts.join('.');
+
+  // check if there's a setting for this path
+  if (Object.prototype.hasOwnProperty.call(expanded, path)) {
+    return expanded[path];
+  }
+
+  // check if all paths are collapsed
+  if (collapsed === true) {
+    return false;
+  }
+
+  // by default all items are expanded
+  return true;
 }
+
+function dataInspectorPropsAreEqual(
+  props: DataInspectorProps,
+  nextProps: DataInspectorProps,
+) {
+  // Optimization: it would be much faster to not pass the expanded tree
+  // down the tree, but rather introduce an ExpandStateManager, and subscribe per node
+
+  // check if any expanded paths effect this subtree
+  if (nextProps.expanded !== props.expanded) {
+    const path = !nextProps.name
+      ? '' // root
+      : !nextProps.parentPath.length
+      ? nextProps.name // root element
+      : nextProps.parentPath.join('.') + '.' + nextProps.name;
+
+    // we are being collapsed
+    if (props.expanded[path] !== nextProps.expanded[path]) {
+      return false;
+    }
+
+    // one of our children was expande
+    for (const key in nextProps.expanded) {
+      if (key.startsWith(path) === false) {
+        // this key doesn't effect us
+        continue;
+      }
+
+      if (nextProps.expanded[key] !== props.expanded[key]) {
+        return false;
+      }
+    }
+  }
+
+  // basic equality checks for the rest
+  return (
+    nextProps.data === props.data &&
+    nextProps.diff === props.diff &&
+    nextProps.name === props.name &&
+    nextProps.depth === props.depth &&
+    nextProps.parentPath === props.parentPath &&
+    nextProps.onExpanded === props.onExpanded &&
+    nextProps.onDelete === props.onDelete &&
+    nextProps.setValue === props.setValue &&
+    nextProps.collapsed === props.collapsed &&
+    nextProps.expandRoot === props.expandRoot
+  );
+}
+
+function isValueExpandable(data: any) {
+  return (
+    typeof data === 'object' && data !== null && Object.keys(data).length > 0
+  );
+}
+
+export default DataInspector;

desktop/app/src/ui/components/data-inspector/ManagedDataInspector.tsx

@@ -63,6 +63,7 @@ type ManagedDataInspectorState = {
 };
 
 const MAX_RESULTS = 50;
+const EMPTY_ARRAY: any[] = [];
 
 /**
  * Wrapper around `DataInspector` that handles expanded state.
@@ -88,7 +89,7 @@ export default class ManagedDataInspector extends PureComponent<
     nextProps: ManagedDataInspectorProps,
     currentState: ManagedDataInspectorState,
   ) {
-    if (nextProps.filter === currentState.filter) {
+    if (nextProps.filter?.toLowerCase() === currentState.filter) {
       return null;
     }
     if (!nextProps.filter) {
@@ -175,6 +176,9 @@ export default class ManagedDataInspector extends PureComponent<
           expandRoot={this.props.expandRoot}
           collapsed={this.props.filter ? true : this.props.collapsed}
           tooltips={this.props.tooltips}
+          parentPath={EMPTY_ARRAY}
+          depth={0}
+          parentAncestry={EMPTY_ARRAY}
         />
       </HighlightProvider>
     );

desktop/app/src/ui/components/data-inspector/__tests__/DataInspector.node.tsx

@@ -177,3 +177,17 @@ test('can filter for data', async () => {
   await res.findByText(/awesomely/);
   await res.findByText(/json/);
 });
+
+test('can render recursive data for data', async () => {
+  const json = {
+    a: {
+      recursive: undefined as any,
+    },
+  };
+  json.a.recursive = json;
+
+  const res = render(
+    <ManagedDataInspector data={json} collapsed={false} expandRoot />,
+  );
+  await res.findByText(/Recursive/);
+});