From dd2686b253cf6b69b4a5797eaed4fba19aa4fbf1 Mon Sep 17 00:00:00 2001 From: Oli Legat Date: Fri, 10 May 2024 17:04:46 +0100 Subject: [PATCH 1/5] Clean up memory management in ChartContext --- .../src/chart/chartContext.ts | 71 +++++++++---------- .../ag-charts-community/src/util/destroy.ts | 14 ++++ 2 files changed, 47 insertions(+), 38 deletions(-) create mode 100644 packages/ag-charts-community/src/util/destroy.ts diff --git a/packages/ag-charts-community/src/chart/chartContext.ts b/packages/ag-charts-community/src/chart/chartContext.ts index 969c2d1d5f..06955ab7c1 100644 --- a/packages/ag-charts-community/src/chart/chartContext.ts +++ b/packages/ag-charts-community/src/chart/chartContext.ts @@ -2,6 +2,7 @@ import type { ModuleContext } from '../module/moduleContext'; import type { Group } from '../scene/group'; import { Scene } from '../scene/scene'; import { CallbackCache } from '../util/callbackCache'; +import { ObjectDestroyer } from '../util/destroy'; import type { Mutex } from '../util/mutex'; import { AnnotationManager } from './annotation/annotationManager'; import type { ChartService } from './chartService'; @@ -59,6 +60,8 @@ export class ChartContext implements ModuleContext { tooltipManager: TooltipManager; zoomManager: ZoomManager; + private readonly owned: ObjectDestroyer; + constructor( chart: ChartService & { zoomManager: ZoomManager; annotationRoot: Group; keyboard: Keyboard; tooltip: Tooltip }, vars: { @@ -78,48 +81,40 @@ export class ChartContext implements ModuleContext { scene?.setContainer(this.domManager); this.scene = scene ?? new Scene({ pixelRatio: overrideDevicePixelRatio, domManager: this.domManager }); - this.annotationManager = new AnnotationManager(chart.annotationRoot); - this.ariaAnnouncementService = new AriaAnnouncementService(this.scene.canvas.element); - this.chartEventManager = new ChartEventManager(); - this.contextMenuRegistry = new ContextMenuRegistry(); - this.cursorManager = new CursorManager(this.domManager); - this.highlightManager = new HighlightManager(); - this.interactionManager = new InteractionManager(chart.keyboard, this.domManager); - this.keyNavManager = new KeyNavManager(this.interactionManager, this.domManager); - this.focusIndicator = new FocusIndicator(this.domManager); - this.regionManager = new RegionManager(this.interactionManager, this.keyNavManager, this.focusIndicator); - this.toolbarManager = new ToolbarManager(); - this.gestureDetector = new GestureDetector(this.domManager); - this.layoutService = new LayoutService(); - this.updateService = new UpdateService(updateCallback); - this.proxyInteractionService = new ProxyInteractionService(this.updateService, this.focusIndicator); - this.seriesStateManager = new SeriesStateManager(); - this.callbackCache = new CallbackCache(); - - this.animationManager = new AnimationManager(this.interactionManager, updateMutex); - this.animationManager.skip(); - this.animationManager.play(); + this.owned = new ObjectDestroyer( + this.domManager, + (this.annotationManager = new AnnotationManager(chart.annotationRoot)), + (this.ariaAnnouncementService = new AriaAnnouncementService(this.scene.canvas.element)), + (this.chartEventManager = new ChartEventManager()), + (this.contextMenuRegistry = new ContextMenuRegistry()), + (this.cursorManager = new CursorManager(this.domManager)), + (this.highlightManager = new HighlightManager()), + (this.interactionManager = new InteractionManager(chart.keyboard, this.domManager)), + (this.keyNavManager = new KeyNavManager(this.interactionManager, this.domManager)), + (this.focusIndicator = new FocusIndicator(this.domManager)), + (this.regionManager = new RegionManager(this.interactionManager, this.keyNavManager, this.focusIndicator)), + (this.toolbarManager = new ToolbarManager()), + (this.gestureDetector = new GestureDetector(this.domManager)), + (this.layoutService = new LayoutService()), + (this.updateService = new UpdateService(updateCallback)), + (this.proxyInteractionService = new ProxyInteractionService(this.updateService, this.focusIndicator)), + (this.seriesStateManager = new SeriesStateManager()), + (this.callbackCache = new CallbackCache()), + (this.animationManager = this.createAnimationManager(this.interactionManager, updateMutex)), + (this.dataService = new DataService(this.animationManager)), + (this.tooltipManager = new TooltipManager(this.domManager, chart.tooltip)) + ); + } - this.dataService = new DataService(this.animationManager); - this.tooltipManager = new TooltipManager(this.domManager, chart.tooltip); + private createAnimationManager(interactionManager: InteractionManager, updateMutex: Mutex): AnimationManager { + const animationManager = new AnimationManager(interactionManager, updateMutex); + animationManager.skip(); + animationManager.play(); + return animationManager; } destroy() { // chart.ts handles the destruction of the scene and zoomManager. - this.tooltipManager.destroy(); - this.proxyInteractionService.destroy(); - this.regionManager.destroy(); - this.focusIndicator.destroy(); - this.keyNavManager.destroy(); - this.interactionManager.destroy(); - this.animationManager.stop(); - this.animationManager.destroy(); - this.ariaAnnouncementService.destroy(); - this.chartEventManager.destroy(); - this.highlightManager.destroy(); - this.callbackCache.invalidateCache(); - this.animationManager.reset(); - this.syncManager.destroy(); - this.domManager.destroy(); + this.owned.destroy(); } } diff --git a/packages/ag-charts-community/src/util/destroy.ts b/packages/ag-charts-community/src/util/destroy.ts new file mode 100644 index 0000000000..0eb75d58c3 --- /dev/null +++ b/packages/ag-charts-community/src/util/destroy.ts @@ -0,0 +1,14 @@ +type Destroyable = {} | { destroy(): void }; +export class ObjectDestroyer { + private readonly objs: Destroyable[]; + constructor(...objs: Destroyable[]) { + this.objs = objs.reverse(); + } + destroy() { + this.objs.forEach((o) => { + if ('destroy' in o && typeof o.destroy === 'function') { + o.destroy(); + } + }); + } +} From 8ede7332e5f88e28f1d62af34a7345a9f7a14384 Mon Sep 17 00:00:00 2001 From: Oli Legat Date: Mon, 13 May 2024 14:56:42 +0100 Subject: [PATCH 2/5] Fix SonarLint errors --- .../src/chart/chartContext.ts | 44 ++++++++++--------- .../ag-charts-community/src/util/destroy.ts | 2 +- 2 files changed, 25 insertions(+), 21 deletions(-) diff --git a/packages/ag-charts-community/src/chart/chartContext.ts b/packages/ag-charts-community/src/chart/chartContext.ts index 06955ab7c1..ac247424d2 100644 --- a/packages/ag-charts-community/src/chart/chartContext.ts +++ b/packages/ag-charts-community/src/chart/chartContext.ts @@ -81,28 +81,32 @@ export class ChartContext implements ModuleContext { scene?.setContainer(this.domManager); this.scene = scene ?? new Scene({ pixelRatio: overrideDevicePixelRatio, domManager: this.domManager }); + // Sonar does not like us using assignments in expression, however this is intended. + // We want to use assignments so that the Typescript compiler can check that we are not using an + // uninitialised property, but we also want to guarantee that ObjectDestroyer knows the + // initialisation order so that it can destroy the objects in reverse. this.owned = new ObjectDestroyer( this.domManager, - (this.annotationManager = new AnnotationManager(chart.annotationRoot)), - (this.ariaAnnouncementService = new AriaAnnouncementService(this.scene.canvas.element)), - (this.chartEventManager = new ChartEventManager()), - (this.contextMenuRegistry = new ContextMenuRegistry()), - (this.cursorManager = new CursorManager(this.domManager)), - (this.highlightManager = new HighlightManager()), - (this.interactionManager = new InteractionManager(chart.keyboard, this.domManager)), - (this.keyNavManager = new KeyNavManager(this.interactionManager, this.domManager)), - (this.focusIndicator = new FocusIndicator(this.domManager)), - (this.regionManager = new RegionManager(this.interactionManager, this.keyNavManager, this.focusIndicator)), - (this.toolbarManager = new ToolbarManager()), - (this.gestureDetector = new GestureDetector(this.domManager)), - (this.layoutService = new LayoutService()), - (this.updateService = new UpdateService(updateCallback)), - (this.proxyInteractionService = new ProxyInteractionService(this.updateService, this.focusIndicator)), - (this.seriesStateManager = new SeriesStateManager()), - (this.callbackCache = new CallbackCache()), - (this.animationManager = this.createAnimationManager(this.interactionManager, updateMutex)), - (this.dataService = new DataService(this.animationManager)), - (this.tooltipManager = new TooltipManager(this.domManager, chart.tooltip)) + (this.annotationManager = new AnnotationManager(chart.annotationRoot)), // NOSONAR + (this.ariaAnnouncementService = new AriaAnnouncementService(this.scene.canvas.element)), // NOSONAR + (this.chartEventManager = new ChartEventManager()), // NOSONAR + (this.contextMenuRegistry = new ContextMenuRegistry()), // NOSONAR + (this.cursorManager = new CursorManager(this.domManager)), // NOSONAR + (this.highlightManager = new HighlightManager()), // NOSONAR + (this.interactionManager = new InteractionManager(chart.keyboard, this.domManager)), // NOSONAR + (this.keyNavManager = new KeyNavManager(this.interactionManager, this.domManager)), // NOSONAR + (this.focusIndicator = new FocusIndicator(this.domManager)), // NOSONAR + (this.regionManager = new RegionManager(this.interactionManager, this.keyNavManager, this.focusIndicator)), // NOSONAR + (this.toolbarManager = new ToolbarManager()), // NOSONAR + (this.gestureDetector = new GestureDetector(this.domManager)), // NOSONAR + (this.layoutService = new LayoutService()), // NOSONAR + (this.updateService = new UpdateService(updateCallback)), // NOSONAR + (this.proxyInteractionService = new ProxyInteractionService(this.updateService, this.focusIndicator)), // NOSONAR + (this.seriesStateManager = new SeriesStateManager()), // NOSONAR + (this.callbackCache = new CallbackCache()), // NOSONAR + (this.animationManager = this.createAnimationManager(this.interactionManager, updateMutex)), // NOSONAR + (this.dataService = new DataService(this.animationManager)), // NOSONAR + (this.tooltipManager = new TooltipManager(this.domManager, chart.tooltip)) // NOSONAR ); } diff --git a/packages/ag-charts-community/src/util/destroy.ts b/packages/ag-charts-community/src/util/destroy.ts index 0eb75d58c3..9b8bf54f88 100644 --- a/packages/ag-charts-community/src/util/destroy.ts +++ b/packages/ag-charts-community/src/util/destroy.ts @@ -2,7 +2,7 @@ type Destroyable = {} | { destroy(): void }; export class ObjectDestroyer { private readonly objs: Destroyable[]; constructor(...objs: Destroyable[]) { - this.objs = objs.reverse(); + this.objs = [...objs].reverse(); } destroy() { this.objs.forEach((o) => { From 132151c72013485bfc90594de6b9bcc5889e2e71 Mon Sep 17 00:00:00 2001 From: Oli Legat Date: Tue, 14 May 2024 11:09:17 +0100 Subject: [PATCH 3/5] CR: Add empty destroy() methods --- .../src/chart/interaction/contextMenuRegistry.ts | 2 ++ .../ag-charts-community/src/chart/interaction/cursorManager.ts | 2 ++ .../ag-charts-community/src/chart/series/seriesStateManager.ts | 2 ++ packages/ag-charts-community/src/util/callbackCache.ts | 2 ++ packages/ag-charts-community/src/util/destroy.ts | 2 +- 5 files changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/ag-charts-community/src/chart/interaction/contextMenuRegistry.ts b/packages/ag-charts-community/src/chart/interaction/contextMenuRegistry.ts index b643e70d68..5e43997a58 100644 --- a/packages/ag-charts-community/src/chart/interaction/contextMenuRegistry.ts +++ b/packages/ag-charts-community/src/chart/interaction/contextMenuRegistry.ts @@ -16,6 +16,8 @@ export class ContextMenuRegistry { private readonly defaultActions: Array = []; private readonly disabledActions: Set = new Set(); + destroy() {} + public filterActions(region: string): ContextMenuAction[] { return this.defaultActions.filter((action) => ['all', region].includes(action.region)); } diff --git a/packages/ag-charts-community/src/chart/interaction/cursorManager.ts b/packages/ag-charts-community/src/chart/interaction/cursorManager.ts index c90aa936f3..83ff397319 100644 --- a/packages/ag-charts-community/src/chart/interaction/cursorManager.ts +++ b/packages/ag-charts-community/src/chart/interaction/cursorManager.ts @@ -24,6 +24,8 @@ export class CursorManager { constructor(private readonly domManager: DOMManager) {} + destroy() {} + public updateCursor(callerId: string, style?: string) { this.stateTracker.set(callerId, style); this.domManager.updateCursor(this.stateTracker.stateValue()!); diff --git a/packages/ag-charts-community/src/chart/series/seriesStateManager.ts b/packages/ag-charts-community/src/chart/series/seriesStateManager.ts index dfb15272d7..2fd1cfd769 100644 --- a/packages/ag-charts-community/src/chart/series/seriesStateManager.ts +++ b/packages/ag-charts-community/src/chart/series/seriesStateManager.ts @@ -15,6 +15,8 @@ export class SeriesStateManager { }; } = {}; + destroy() {} + public registerSeries({ id, seriesGrouping, diff --git a/packages/ag-charts-community/src/util/callbackCache.ts b/packages/ag-charts-community/src/util/callbackCache.ts index ef4eea6396..4b79427d2a 100644 --- a/packages/ag-charts-community/src/util/callbackCache.ts +++ b/packages/ag-charts-community/src/util/callbackCache.ts @@ -3,6 +3,8 @@ import { Logger } from './logger'; export class CallbackCache { private cache: WeakMap> = new WeakMap(); + destroy() {} + call any>(fn: F, ...params: Parameters): ReturnType | undefined { let serialisedParams: string; let paramCache = this.cache.get(fn); diff --git a/packages/ag-charts-community/src/util/destroy.ts b/packages/ag-charts-community/src/util/destroy.ts index 9b8bf54f88..935bef9c01 100644 --- a/packages/ag-charts-community/src/util/destroy.ts +++ b/packages/ag-charts-community/src/util/destroy.ts @@ -1,4 +1,4 @@ -type Destroyable = {} | { destroy(): void }; +type Destroyable = { destroy(): void }; export class ObjectDestroyer { private readonly objs: Destroyable[]; constructor(...objs: Destroyable[]) { From 3af32621e5b3e457ab9a64d21062770637d4e7d9 Mon Sep 17 00:00:00 2001 From: Oli Legat Date: Tue, 14 May 2024 11:13:23 +0100 Subject: [PATCH 4/5] Specify that these implement destroy() as part of an interface --- .../src/chart/interaction/contextMenuRegistry.ts | 4 +++- .../src/chart/interaction/cursorManager.ts | 3 ++- .../src/chart/series/seriesStateManager.ts | 4 +++- packages/ag-charts-community/src/util/callbackCache.ts | 3 ++- packages/ag-charts-community/src/util/destroy.ts | 3 ++- 5 files changed, 12 insertions(+), 5 deletions(-) diff --git a/packages/ag-charts-community/src/chart/interaction/contextMenuRegistry.ts b/packages/ag-charts-community/src/chart/interaction/contextMenuRegistry.ts index 5e43997a58..7917483752 100644 --- a/packages/ag-charts-community/src/chart/interaction/contextMenuRegistry.ts +++ b/packages/ag-charts-community/src/chart/interaction/contextMenuRegistry.ts @@ -1,3 +1,5 @@ +import type { Destroyable } from "../../util/destroy"; + export type ContextMenuAction = { id?: string; label: string; @@ -12,7 +14,7 @@ export type ContextMenuActionParams = { event: MouseEvent; }; -export class ContextMenuRegistry { +export class ContextMenuRegistry implements Destroyable { private readonly defaultActions: Array = []; private readonly disabledActions: Set = new Set(); diff --git a/packages/ag-charts-community/src/chart/interaction/cursorManager.ts b/packages/ag-charts-community/src/chart/interaction/cursorManager.ts index 83ff397319..775b518f1b 100644 --- a/packages/ag-charts-community/src/chart/interaction/cursorManager.ts +++ b/packages/ag-charts-community/src/chart/interaction/cursorManager.ts @@ -1,3 +1,4 @@ +import type { Destroyable } from '../../util/destroy'; import { StateTracker } from '../../util/stateTracker'; import type { DOMManager } from '../dom/domManager'; @@ -19,7 +20,7 @@ export enum Cursor { * Manages the cursor styling for an element. Tracks the requested styling from distinct * dependents and handles conflicting styling requests. */ -export class CursorManager { +export class CursorManager implements Destroyable { private readonly stateTracker = new StateTracker('default'); constructor(private readonly domManager: DOMManager) {} diff --git a/packages/ag-charts-community/src/chart/series/seriesStateManager.ts b/packages/ag-charts-community/src/chart/series/seriesStateManager.ts index 2fd1cfd769..7289ef1c6b 100644 --- a/packages/ag-charts-community/src/chart/series/seriesStateManager.ts +++ b/packages/ag-charts-community/src/chart/series/seriesStateManager.ts @@ -1,3 +1,5 @@ +import type { Destroyable } from "../../util/destroy"; + export type SeriesGrouping = { groupIndex: number; groupCount: number; @@ -5,7 +7,7 @@ export type SeriesGrouping = { stackCount: number; }; -export class SeriesStateManager { +export class SeriesStateManager implements Destroyable { private readonly groups: { [type: string]: { [id: string]: { diff --git a/packages/ag-charts-community/src/util/callbackCache.ts b/packages/ag-charts-community/src/util/callbackCache.ts index 4b79427d2a..3f2f9448ac 100644 --- a/packages/ag-charts-community/src/util/callbackCache.ts +++ b/packages/ag-charts-community/src/util/callbackCache.ts @@ -1,6 +1,7 @@ +import type { Destroyable } from './destroy'; import { Logger } from './logger'; -export class CallbackCache { +export class CallbackCache implements Destroyable { private cache: WeakMap> = new WeakMap(); destroy() {} diff --git a/packages/ag-charts-community/src/util/destroy.ts b/packages/ag-charts-community/src/util/destroy.ts index 935bef9c01..517d549096 100644 --- a/packages/ag-charts-community/src/util/destroy.ts +++ b/packages/ag-charts-community/src/util/destroy.ts @@ -1,4 +1,5 @@ -type Destroyable = { destroy(): void }; +export type Destroyable = { destroy(): void }; + export class ObjectDestroyer { private readonly objs: Destroyable[]; constructor(...objs: Destroyable[]) { From 9c86fc5fab96d3390f2294806dbe34702f307598 Mon Sep 17 00:00:00 2001 From: Oli Legat Date: Tue, 14 May 2024 11:28:55 +0100 Subject: [PATCH 5/5] nx format --- .../src/chart/interaction/contextMenuRegistry.ts | 2 +- .../ag-charts-community/src/chart/series/seriesStateManager.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/ag-charts-community/src/chart/interaction/contextMenuRegistry.ts b/packages/ag-charts-community/src/chart/interaction/contextMenuRegistry.ts index 7917483752..f03983c8ff 100644 --- a/packages/ag-charts-community/src/chart/interaction/contextMenuRegistry.ts +++ b/packages/ag-charts-community/src/chart/interaction/contextMenuRegistry.ts @@ -1,4 +1,4 @@ -import type { Destroyable } from "../../util/destroy"; +import type { Destroyable } from '../../util/destroy'; export type ContextMenuAction = { id?: string; diff --git a/packages/ag-charts-community/src/chart/series/seriesStateManager.ts b/packages/ag-charts-community/src/chart/series/seriesStateManager.ts index 7289ef1c6b..bc0a473453 100644 --- a/packages/ag-charts-community/src/chart/series/seriesStateManager.ts +++ b/packages/ag-charts-community/src/chart/series/seriesStateManager.ts @@ -1,4 +1,4 @@ -import type { Destroyable } from "../../util/destroy"; +import type { Destroyable } from '../../util/destroy'; export type SeriesGrouping = { groupIndex: number;