From 49b129b110b6927405999b53b1d84a6bbddb1ba3 Mon Sep 17 00:00:00 2001 From: Leon Sorokin Date: Wed, 18 Aug 2021 13:43:53 -0500 Subject: [PATCH] GraphNG: fix mem leaks & avoid plot re-inits (#38017) * State timeline: fix mem leak caused by excessive plot re-init * Update PlotTooltipInterpolator type * Do not reference config object in the setCursor hook * fix excessive BarChart re-init caused by shallow diff of text config * one less error Co-authored-by: Dominik Prokop Co-authored-by: Ryan McKinley --- .../src/components/GraphNG/GraphNG.tsx | 15 +++++-- .../uPlot/config/UPlotConfigBuilder.ts | 11 ++--- .../uPlot/plugins/TooltipPlugin.tsx | 36 ++++++++------- .../grafana-ui/src/components/uPlot/types.ts | 5 ++- .../app/plugins/panel/barchart/BarChart.tsx | 10 ++++- public/app/plugins/panel/barchart/bars.ts | 45 +++++++++---------- .../app/plugins/panel/state-timeline/utils.ts | 17 +++++-- 7 files changed, 86 insertions(+), 53 deletions(-) diff --git a/packages/grafana-ui/src/components/GraphNG/GraphNG.tsx b/packages/grafana-ui/src/components/GraphNG/GraphNG.tsx index 304e61a8e67..4fd09cd2387 100755 --- a/packages/grafana-ui/src/components/GraphNG/GraphNG.tsx +++ b/packages/grafana-ui/src/components/GraphNG/GraphNG.tsx @@ -27,6 +27,11 @@ import { UPlotChart } from '../uPlot/Plot'; */ export const FIXED_UNIT = '__fixed'; +/** + * @internal -- not a public API + */ +export type PropDiffFn = (prev: T, next: T) => boolean; + export interface GraphNGProps extends Themeable2 { frames: DataFrame[]; structureRev?: number; // a number that will change when the frames[] structure changes @@ -39,14 +44,18 @@ export interface GraphNGProps extends Themeable2 { onLegendClick?: (event: GraphNGLegendEvent) => void; children?: (builder: UPlotConfigBuilder, alignedFrame: DataFrame) => React.ReactNode; prepConfig: (alignedFrame: DataFrame, allFrames: DataFrame[], getTimeRange: () => TimeRange) => UPlotConfigBuilder; - propsToDiff?: string[]; + propsToDiff?: Array; preparePlotFrame?: (frames: DataFrame[], dimFields: XYFieldMatchers) => DataFrame; renderLegend: (config: UPlotConfigBuilder) => React.ReactElement | null; } -function sameProps(prevProps: any, nextProps: any, propsToDiff: string[] = []) { +function sameProps(prevProps: any, nextProps: any, propsToDiff: Array = []) { for (const propName of propsToDiff) { - if (nextProps[propName] !== prevProps[propName]) { + if (typeof propName === 'function') { + if (!propName(prevProps, nextProps)) { + return false; + } + } else if (nextProps[propName] !== prevProps[propName]) { return false; } } diff --git a/packages/grafana-ui/src/components/uPlot/config/UPlotConfigBuilder.ts b/packages/grafana-ui/src/components/uPlot/config/UPlotConfigBuilder.ts index 7ab3a654cac..16cedfa9d2d 100644 --- a/packages/grafana-ui/src/components/uPlot/config/UPlotConfigBuilder.ts +++ b/packages/grafana-ui/src/components/uPlot/config/UPlotConfigBuilder.ts @@ -49,11 +49,8 @@ export class UPlotConfigBuilder { private frame: DataFrame | undefined = undefined; // to prevent more than one threshold per scale private thresholds: Record = {}; - /** - * Custom handler for closest datapoint and series lookup. Technicaly returns uPlots setCursor hook - * that sets tooltips state. - */ - tooltipInterpolator: PlotTooltipInterpolator | undefined = undefined; + // Custom handler for closest datapoint and series lookup + private tooltipInterpolator: PlotTooltipInterpolator | undefined = undefined; prepData: PrepData | undefined = undefined; @@ -154,6 +151,10 @@ export class UPlotConfigBuilder { this.tooltipInterpolator = interpolator; } + getTooltipInterpolator() { + return this.tooltipInterpolator; + } + setPrepData(prepData: PrepData) { this.prepData = (frame) => { this.frame = frame; diff --git a/packages/grafana-ui/src/components/uPlot/plugins/TooltipPlugin.tsx b/packages/grafana-ui/src/components/uPlot/plugins/TooltipPlugin.tsx index 1884dce43cf..4fac7b28959 100644 --- a/packages/grafana-ui/src/components/uPlot/plugins/TooltipPlugin.tsx +++ b/packages/grafana-ui/src/components/uPlot/plugins/TooltipPlugin.tsx @@ -93,25 +93,31 @@ export const TooltipPlugin: React.FC = ({ // Add uPlot hooks to the config, or re-add when the config changed useLayoutEffect(() => { - if (config.tooltipInterpolator) { + const tooltipInterpolator = config.getTooltipInterpolator(); + if (tooltipInterpolator) { // Custom toolitp positioning config.addHook('setCursor', (u) => { - config.tooltipInterpolator!(setFocusedSeriesIdx, setFocusedPointIdx, (clear) => { - if (clear) { - setCoords(null); - return; - } + tooltipInterpolator( + setFocusedSeriesIdx, + setFocusedPointIdx, + (clear) => { + if (clear) { + setCoords(null); + return; + } - const bbox = plotCtx.getCanvasBoundingBox(); - if (!bbox) { - return; - } + const bbox = plotCtx.getCanvasBoundingBox(); + if (!bbox) { + return; + } - const { x, y } = positionTooltip(u, bbox); - if (x !== undefined && y !== undefined) { - setCoords({ x, y }); - } - })(u); + const { x, y } = positionTooltip(u, bbox); + if (x !== undefined && y !== undefined) { + setCoords({ x, y }); + } + }, + u + ); }); } else { config.addHook('setLegend', (u) => { diff --git a/packages/grafana-ui/src/components/uPlot/types.ts b/packages/grafana-ui/src/components/uPlot/types.ts index f63f3b36adf..ef045315caf 100755 --- a/packages/grafana-ui/src/components/uPlot/types.ts +++ b/packages/grafana-ui/src/components/uPlot/types.ts @@ -34,8 +34,9 @@ export abstract class PlotConfigBuilder { export type PlotTooltipInterpolator = ( updateActiveSeriesIdx: (sIdx: number | null) => void, updateActiveDatapointIdx: (dIdx: number | null) => void, - updateTooltipPosition: (clear?: boolean) => void -) => (u: uPlot) => void; + updateTooltipPosition: (clear?: boolean) => void, + u: uPlot +) => void; export interface PlotSelection { min: number; diff --git a/public/app/plugins/panel/barchart/BarChart.tsx b/public/app/plugins/panel/barchart/BarChart.tsx index 2a862b763d3..95fe6332cf6 100644 --- a/public/app/plugins/panel/barchart/BarChart.tsx +++ b/public/app/plugins/panel/barchart/BarChart.tsx @@ -12,6 +12,7 @@ import { } from '@grafana/ui'; import { BarChartOptions } from './types'; import { preparePlotConfigBuilder, preparePlotFrame } from './utils'; +import { PropDiffFn } from '../../../../../packages/grafana-ui/src/components/GraphNG/GraphNG'; /** * @alpha @@ -20,7 +21,14 @@ export interface BarChartProps extends BarChartOptions, Omit {} -const propsToDiff: string[] = ['orientation', 'barWidth', 'groupWidth', 'stacking', 'showValue', 'text']; +const propsToDiff: Array = [ + 'orientation', + 'barWidth', + 'groupWidth', + 'stacking', + 'showValue', + (prev: BarChartProps, next: BarChartProps) => next.text?.valueSize === prev.text?.valueSize, +]; export const BarChart: React.FC = (props) => { const theme = useTheme2(); diff --git a/public/app/plugins/panel/barchart/bars.ts b/public/app/plugins/panel/barchart/bars.ts index b1fb04fd246..813b1298d68 100644 --- a/public/app/plugins/panel/barchart/bars.ts +++ b/public/app/plugins/panel/barchart/bars.ts @@ -272,22 +272,22 @@ export function getConfig(opts: BarsOptions, theme: GrafanaTheme2) { const interpolateTooltip: PlotTooltipInterpolator = ( updateActiveSeriesIdx, updateActiveDatapointIdx, - updateTooltipPosition + updateTooltipPosition, + u ) => { - return (u: uPlot) => { - let found: Rect | undefined; - let cx = u.cursor.left! * devicePixelRatio; - let cy = u.cursor.top! * devicePixelRatio; + let found: Rect | undefined; + let cx = u.cursor.left! * devicePixelRatio; + let cy = u.cursor.top! * devicePixelRatio; - qt.get(cx, cy, 1, 1, (o) => { - if (pointWithin(cx, cy, o.x, o.y, o.x + o.w, o.y + o.h)) { - found = o; - } - }); + qt.get(cx, cy, 1, 1, (o) => { + if (pointWithin(cx, cy, o.x, o.y, o.x + o.w, o.y + o.h)) { + found = o; + } + }); - if (found) { - // prettier-ignore - if (found !== hovered) { + if (found) { + // prettier-ignore + if (found !== hovered) { barMark.style.display = ''; barMark.style.left = found.x / devicePixelRatio + 'px'; barMark.style.top = found.y / devicePixelRatio + 'px'; @@ -298,16 +298,15 @@ export function getConfig(opts: BarsOptions, theme: GrafanaTheme2) { updateActiveDatapointIdx(hovered.didx); updateTooltipPosition(); } - } else if (hovered !== undefined) { - updateActiveSeriesIdx(hovered!.sidx); - updateActiveDatapointIdx(hovered!.didx); - updateTooltipPosition(); - hovered = undefined; - barMark.style.display = 'none'; - } else { - updateTooltipPosition(true); - } - }; + } else if (hovered !== undefined) { + updateActiveSeriesIdx(hovered!.sidx); + updateActiveDatapointIdx(hovered!.didx); + updateTooltipPosition(); + hovered = undefined; + barMark.style.display = 'none'; + } else { + updateTooltipPosition(true); + } }; let alignedTotals: AlignedData | null = null; diff --git a/public/app/plugins/panel/state-timeline/utils.ts b/public/app/plugins/panel/state-timeline/utils.ts index e28e39f8034..dacdd01b271 100644 --- a/public/app/plugins/panel/state-timeline/utils.ts +++ b/public/app/plugins/panel/state-timeline/utils.ts @@ -126,7 +126,7 @@ export const preparePlotConfigBuilder: UPlotConfigPrepFn = ({ updateActiveSeriesIdx, updateActiveDatapointIdx, updateTooltipPosition - ) => (u: uPlot) => { + ) => { if (shouldChangeHover) { if (hoveredSeriesIdx != null) { updateActiveSeriesIdx(hoveredSeriesIdx); @@ -271,7 +271,7 @@ export function prepareTimelineFields( let isTimeseries = false; let changed = false; const fields: Field[] = []; - for (const field of frame.fields) { + for (let field of frame.fields) { switch (field.type) { case FieldType.time: isTimeseries = true; @@ -281,8 +281,17 @@ export function prepareTimelineFields( case FieldType.number: case FieldType.boolean: case FieldType.string: - // magic value for join() to leave nulls alone - (field.config.custom = field.config.custom ?? {}).spanNulls = -1; + field = { + ...field, + config: { + ...field.config, + custom: { + ...field.config.custom, + // magic value for join() to leave nulls alone + spanNulls: -1, + }, + }, + }; if (mergeValues) { let merged = unsetSameFutureValues(field.values.toArray());