perf(flamegraph): don't convert to graphviz format unnecessarily (#1834)

* perf(flamegraph): don't convert to graphviz data unnecessarily
grafana · Feb 3, 2023 · 8f78e54 · 8f78e54
1 parent f4101e1
commit 8f78e54
Show file tree

Hide file tree

Showing 4 changed files with 121 additions and 115 deletions.
diff --git a/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/GraphVizPane.tsx b/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/GraphVizPane.tsx
@@ -0,0 +1,64 @@
+import type { Flamebearer } from '@pyroscope/models/src';
+import React, { useMemo } from 'react';
+import toGraphviz from '../../convert/toGraphviz';
+import styles from './GraphVizPanel.module.scss';
+
+// this is to make sure that graphviz-react is not used in node.js
+let Graphviz = (obj: IGraphvizProps) => {
+  if (obj) {
+    return null;
+  }
+  return null;
+};
+interface IGraphvizProps {
+  dot: string;
+  options?: object;
+  className?: string;
+}
+
+if (typeof process === 'undefined') {
+  /* eslint-disable global-require */
+  Graphviz = require('graphviz-react').Graphviz;
+}
+
+interface GraphVizPaneProps {
+  flamebearer: Flamebearer;
+}
+export function GraphVizPane({ flamebearer }: GraphVizPaneProps) {
+  // TODO(@petethepig): I don't understand what's going on with types here
+  //   need to fix at some point
+  const fb = flamebearer as ShamefulAny;
+
+  // flamebearer
+  const dot = fb.metadata?.format && fb.flamebearer?.levels;
+
+  // Graphviz doesn't update position and scale value on rerender
+  // so image sometimes moves out of the screen
+  // to fix it we remounting graphViz component by updating key
+  const key = `graphviz-pane-${fb?.appName || String(new Date().valueOf())}`;
+  const dotValue = useMemo(() => {
+    return toGraphviz(fb);
+  }, [JSON.stringify(fb)]);
+
+  return (
+    <div className={styles.graphVizPane} key={key}>
+      {dot ? (
+        <Graphviz
+          // options https://github.com/magjac/d3-graphviz#supported-options
+          options={{
+            zoom: true,
+            width: '150%',
+            height: '100%',
+            scale: 1,
+            // 'true' by default, but causes warning
+            // https://github.com/magjac/d3-graphviz/blob/master/README.md#defining-the-hpcc-jswasm-script-tag
+            useWorker: false,
+          }}
+          dot={dotValue}
+        />
+      ) : (
+        <div>NO DATA</div>
+      )}
+    </div>
+  );
+}
diff --git a/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/GraphVizPanel.module.scss b/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/GraphVizPanel.module.scss
@@ -0,0 +1,55 @@
+.graphVizPane {
+  display: flex;
+  flex: 1;
+
+  // hacky better than !important
+  &#{&} {
+    margin-right: 0;
+    border: 1px solid var(--ps-ui-border);
+  }
+
+  div[id^='graphviz'] {
+    width: 100%;
+    overflow: hidden;
+
+    :global {
+      .graph > polygon {
+        // graphviz overlay
+        fill: none;
+      }
+
+      .node {
+        polygon {
+          // node box
+          // stroke: var(--ps-fl-toolbar-btn-bg);
+        }
+
+        text[text-anchor='middle'] {
+          // node caption
+          // fill: var(--ps-toolbar-icon-color);
+        }
+      }
+
+      .edge {
+        text[text-anchor='middle'] {
+          // edge caption
+          fill: var(--ps-toolbar-icon-color);
+          // fill: red;
+        }
+
+        a {
+          path {
+            // arrow body
+            // stroke: var(--ps-fl-toolbar-btn-bg);
+          }
+
+          polygon {
+            // arrow head
+            // stroke: var(--ps-fl-toolbar-btn-bg);
+            // fill: var(--ps-fl-toolbar-btn-bg);
+          }
+        }
+      }
+    }
+  }
+}
diff --git a/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphRenderer.tsx b/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphRenderer.tsx
@@ -21,30 +21,13 @@ import {
   calleesProfile,
   callersProfile,
 } from '../convert/sandwichViewProfiles';
-import toGraphviz from '../convert/toGraphviz';
 import { DefaultPalette } from './FlameGraphComponent/colorPalette';
 import styles from './FlamegraphRenderer.module.scss';
 import PyroscopeLogo from '../logo-v3-small.svg';
 import decode from './decode';
 import { FitModes } from '../fitMode/fitMode';
 import { ViewTypes } from './FlameGraphComponent/viewTypes';
-
-interface IGraphvizProps {
-  dot: string;
-  options?: object;
-  className?: string;
-}
-
-// this is to make sure that graphviz-react is not used in node.js
-let Graphviz = (obj: IGraphvizProps) => {
-  if (obj) {
-    return null;
-  }
-  return null;
-};
-if (typeof process === 'undefined') {
-  Graphviz = require('graphviz-react').Graphviz;
-}
+import { GraphVizPane } from './FlameGraphComponent/GraphVizPane';
 
 // Still support old flamebearer format
 // But prefer the new 'profile' one
@@ -560,54 +543,14 @@ class FlameGraphRenderer extends Component<
     //   //  rightTicks?: number;
     // } & addTicks;
 
-    const graphvizPane = (() => {
-      // TODO(@petethepig): I don't understand what's going on with types here
-      //   need to fix at some point
-      const flamebearer = this.state.flamebearer as ShamefulAny;
-      // flamebearer
-      const dot =
-        flamebearer.metadata?.format && flamebearer.flamebearer?.levels
-          ? toGraphviz(flamebearer)
-          : null;
-
-      // Graphviz doesn't update position and scale value on rerender
-      // so image sometimes moves out of the screen
-      // to fix it we remounting graphViz component by updating key
-      const key = `graphviz-pane-${
-        flamebearer?.appName || String(new Date().valueOf())
-      }`;
-
-      return (
-        <div className={styles.graphVizPane} key={key}>
-          {dot ? (
-            <Graphviz
-              // options https://github.com/magjac/d3-graphviz#supported-options
-              options={{
-                zoom: true,
-                width: '150%',
-                height: '100%',
-                scale: 1,
-                // 'true' by default, but causes warning
-                // https://github.com/magjac/d3-graphviz/blob/master/README.md#defining-the-hpcc-jswasm-script-tag
-                useWorker: false,
-              }}
-              dot={dot}
-            />
-          ) : (
-            <div>NO DATA</div>
-          )}
-        </div>
-      );
-    })();
-
     const dataUnavailable =
       !this.state.flamebearer || this.state.flamebearer.names.length <= 1;
     const panes = decidePanesOrder(
       this.state.view,
       flameGraphPane,
       tablePane,
       sandwichPane,
-      graphvizPane
+      <GraphVizPane flamebearer={this.state.flamebearer} />
     );
 
     return (

diff --git a/packages/pyroscope-flamegraph/src/FlameGraph/FlamegraphRenderer.module.scss b/packages/pyroscope-flamegraph/src/FlameGraph/FlamegraphRenderer.module.scss
@@ -91,59 +91,3 @@
     text-align: right;
   }
 }
-
-.graphVizPane {
-  display: flex;
-  flex: 1;
-
-  // hacky better than !important
-  &#{&} {
-    margin-right: 0;
-    border: 1px solid var(--ps-ui-border);
-  }
-
-  div[id^='graphviz'] {
-    width: 100%;
-    overflow: hidden;
-
-    :global {
-      .graph > polygon {
-        // graphviz overlay
-        fill: none;
-      }
-
-      .node {
-        polygon {
-          // node box
-          // stroke: var(--ps-fl-toolbar-btn-bg);
-        }
-
-        text[text-anchor='middle'] {
-          // node caption
-          // fill: var(--ps-toolbar-icon-color);
-        }
-      }
-
-      .edge {
-        text[text-anchor='middle'] {
-          // edge caption
-          fill: var(--ps-toolbar-icon-color);
-          // fill: red;
-        }
-
-        a {
-          path {
-            // arrow body
-            // stroke: var(--ps-fl-toolbar-btn-bg);
-          }
-
-          polygon {
-            // arrow head
-            // stroke: var(--ps-fl-toolbar-btn-bg);
-            // fill: var(--ps-fl-toolbar-btn-bg);
-          }
-        }
-      }
-    }
-  }
-}