From 038138e6b697709cdd1ad810ae79b3e014790792 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Mon, 9 Dec 2024 14:27:32 -0600 Subject: [PATCH 1/7] optimize main thread to worker transfer when recoloring --- app/packages/looker/src/lookers/abstract.ts | 55 ++++++++++++++----- app/packages/looker/src/overlays/base.ts | 4 +- app/packages/looker/src/overlays/detection.ts | 6 ++ app/packages/looker/src/overlays/heatmap.ts | 9 ++- .../looker/src/overlays/segmentation.ts | 9 ++- .../looker/src/worker/disk-overlay-decoder.ts | 6 +- 6 files changed, 68 insertions(+), 21 deletions(-) diff --git a/app/packages/looker/src/lookers/abstract.ts b/app/packages/looker/src/lookers/abstract.ts index 3eca774de8..81b9c9ab1e 100644 --- a/app/packages/looker/src/lookers/abstract.ts +++ b/app/packages/looker/src/lookers/abstract.ts @@ -23,7 +23,7 @@ import { import { Events } from "../elements/base"; import { COMMON_SHORTCUTS, LookerElement } from "../elements/common"; import { ClassificationsOverlay, loadOverlays } from "../overlays"; -import { CONTAINS, Overlay } from "../overlays/base"; +import { CONTAINS, LabelMask, Overlay } from "../overlays/base"; import processOverlays from "../processOverlays"; import { BaseState, @@ -515,7 +515,29 @@ export abstract class AbstractLooker< abstract updateOptions(options: Partial): void; updateSample(sample: Sample) { - this.loadSample(sample); + // collect any mask targets array buffer that overalys might have + // we'll transfer that to the worker instead of copying it + const arrayBuffers: ArrayBuffer[] = []; + + for (const overlay of this.pluckedOverlays ?? []) { + // we paint overlays again, so cleanup the old ones + // this helps prevent memory leaks from, for instance, dangling ImageBitmaps + overlay.cleanup(); + + let overlayData: LabelMask = null; + + if ("mask" in overlay.label) { + overlayData = overlay.label.mask as LabelMask; + } else if ("map" in overlay.label) { + overlayData = overlay.label.map as LabelMask; + } + + if (overlayData?.data?.buffer) { + arrayBuffers.push(overlayData.data.buffer); + } + } + + this.loadSample(sample, arrayBuffers); } getSample(): Promise { @@ -698,7 +720,7 @@ export abstract class AbstractLooker< ); } - private loadSample(sample: Sample) { + private loadSample(sample: Sample, transfer: Transferable[] = []) { const messageUUID = uuid(); const labelsWorker = getLabelsWorker(); @@ -719,18 +741,21 @@ export abstract class AbstractLooker< labelsWorker.addEventListener("message", listener); - labelsWorker.postMessage({ - sample: sample as ProcessSample["sample"], - method: "processSample", - coloring: this.state.options.coloring, - customizeColorSetting: this.state.options.customizeColorSetting, - colorscale: this.state.options.colorscale, - labelTagColors: this.state.options.labelTagColors, - selectedLabelTags: this.state.options.selectedLabelTags, - sources: this.state.config.sources, - schema: this.state.config.fieldSchema, - uuid: messageUUID, - } as ProcessSample); + labelsWorker.postMessage( + { + sample: sample as ProcessSample["sample"], + method: "processSample", + coloring: this.state.options.coloring, + customizeColorSetting: this.state.options.customizeColorSetting, + colorscale: this.state.options.colorscale, + labelTagColors: this.state.options.labelTagColors, + selectedLabelTags: this.state.options.selectedLabelTags, + sources: this.state.config.sources, + schema: this.state.config.fieldSchema, + uuid: messageUUID, + } as ProcessSample, + transfer + ); } } diff --git a/app/packages/looker/src/overlays/base.ts b/app/packages/looker/src/overlays/base.ts index a3ec867766..9b433e9400 100644 --- a/app/packages/looker/src/overlays/base.ts +++ b/app/packages/looker/src/overlays/base.ts @@ -42,6 +42,7 @@ export interface SelectData { export type LabelMask = { bitmap?: ImageBitmap; + closedBitmapDims?: { width: number; height: number }; data?: OverlayMask; }; @@ -67,6 +68,7 @@ export interface Overlay> { draw(ctx: CanvasRenderingContext2D, state: State): void; isShown(state: Readonly): boolean; field?: string; + label?: BaseLabel; containsPoint(state: Readonly): CONTAINS; getMouseDistance(state: Readonly): number; getPointInfo(state: Readonly): any; @@ -82,7 +84,7 @@ export abstract class CoordinateOverlay< > implements Overlay { readonly field: string; - protected label: Label; + readonly label: Label; constructor(field: string, label: Label) { this.field = field; diff --git a/app/packages/looker/src/overlays/detection.ts b/app/packages/looker/src/overlays/detection.ts index 137beae7b4..d5b80d9873 100644 --- a/app/packages/looker/src/overlays/detection.ts +++ b/app/packages/looker/src/overlays/detection.ts @@ -263,8 +263,14 @@ export default class DetectionOverlay< public cleanup(): void { if (this.label.mask?.bitmap) { + // store height and width in bitmap object since it might be used again + const height = this.label.mask.bitmap.height; + const width = this.label.mask.bitmap.width; + this.label.mask?.bitmap.close(); this.label.mask.bitmap = null; + + this.label.mask.closedBitmapDims = { width, height }; } } } diff --git a/app/packages/looker/src/overlays/heatmap.ts b/app/packages/looker/src/overlays/heatmap.ts index e8e8817643..c1be209ed5 100644 --- a/app/packages/looker/src/overlays/heatmap.ts +++ b/app/packages/looker/src/overlays/heatmap.ts @@ -39,7 +39,7 @@ export default class HeatmapOverlay implements Overlay { readonly field: string; - private label: HeatmapLabel; + readonly label: HeatmapLabel; private targets?: TypedArray; private readonly range: [number, number]; @@ -208,7 +208,14 @@ export default class HeatmapOverlay public cleanup(): void { if (this.label.map?.bitmap) { + // store height and width in bitmap object since it might be used again + const height = this.label.map.bitmap.height; + const width = this.label.map.bitmap.width; + this.label.map?.bitmap.close(); + this.label.map.bitmap = null; + + this.label.map.closedBitmapDims = { width, height }; } } } diff --git a/app/packages/looker/src/overlays/segmentation.ts b/app/packages/looker/src/overlays/segmentation.ts index a4cb098254..3218db80a6 100644 --- a/app/packages/looker/src/overlays/segmentation.ts +++ b/app/packages/looker/src/overlays/segmentation.ts @@ -30,7 +30,7 @@ export default class SegmentationOverlay implements Overlay { readonly field: string; - private label: SegmentationLabel; + readonly label: SegmentationLabel; private targets?: TypedArray; private isRgbMaskTargets = false; @@ -263,7 +263,14 @@ export default class SegmentationOverlay public cleanup(): void { if (this.label.mask?.bitmap) { + // store height and width in bitmap object since it might be used again + const height = this.label.mask.bitmap.height; + const width = this.label.mask.bitmap.width; + this.label.mask?.bitmap.close(); + this.label.mask.bitmap = null; + + this.label.mask.closedBitmapDims = { width, height }; } } } diff --git a/app/packages/looker/src/worker/disk-overlay-decoder.ts b/app/packages/looker/src/worker/disk-overlay-decoder.ts index 8730f74bf0..e2cbf6081b 100644 --- a/app/packages/looker/src/worker/disk-overlay-decoder.ts +++ b/app/packages/looker/src/worker/disk-overlay-decoder.ts @@ -56,11 +56,11 @@ export const decodeOverlayOnDisk = async ( // it's possible we're just re-coloring, in which case re-init mask image and set bitmap to null if ( label[overlayField] && - label[overlayField].bitmap && + label[overlayField].closedBitmapDims && !label[overlayField].image ) { - const height = label[overlayField].bitmap.height; - const width = label[overlayField].bitmap.width; + const height = label[overlayField].closedBitmapDims.height; + const width = label[overlayField].closedBitmapDims.width; label[overlayField].image = new ArrayBuffer(height * width * 4); label[overlayField].bitmap.close(); label[overlayField].bitmap = null; From ece501a06c7b7e4cb3d9554a1f3c683cd61b9ac2 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Mon, 9 Dec 2024 14:34:57 -0600 Subject: [PATCH 2/7] fix typo --- app/packages/looker/src/lookers/abstract.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/packages/looker/src/lookers/abstract.ts b/app/packages/looker/src/lookers/abstract.ts index 81b9c9ab1e..423656e49a 100644 --- a/app/packages/looker/src/lookers/abstract.ts +++ b/app/packages/looker/src/lookers/abstract.ts @@ -515,7 +515,7 @@ export abstract class AbstractLooker< abstract updateOptions(options: Partial): void; updateSample(sample: Sample) { - // collect any mask targets array buffer that overalys might have + // collect any mask targets array buffer that overlays might have // we'll transfer that to the worker instead of copying it const arrayBuffers: ArrayBuffer[] = []; From 89fea41f0829bc58db41bddfd19668dc62b78aea Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Mon, 9 Dec 2024 18:53:49 -0600 Subject: [PATCH 3/7] check if array buffer detached --- app/packages/looker/src/lookers/abstract.ts | 64 +++++++++++++++------ app/packages/looker/src/worker/index.ts | 4 ++ 2 files changed, 50 insertions(+), 18 deletions(-) diff --git a/app/packages/looker/src/lookers/abstract.ts b/app/packages/looker/src/lookers/abstract.ts index 423656e49a..6234ae6082 100644 --- a/app/packages/looker/src/lookers/abstract.ts +++ b/app/packages/looker/src/lookers/abstract.ts @@ -532,12 +532,30 @@ export abstract class AbstractLooker< overlayData = overlay.label.map as LabelMask; } - if (overlayData?.data?.buffer) { - arrayBuffers.push(overlayData.data.buffer); + const buffer = overlayData?.data?.buffer; + + if (!buffer) { + continue; + } + + // check for detached buffer (happens if user is switching colors too fast) + // note: ArrayBuffer.prototype.detached is a new browser API + if (typeof buffer.detached !== "undefined") { + if (buffer.detached) { + // most likely sample is already being processed, skip update + return; + } else { + arrayBuffers.push(buffer); + } + } else { + // hope we don't run into this edge case (old browser) + // if we do, we'll just copy the buffer + // might get a DataCloneError if user is switching colors too fast + arrayBuffers.push(buffer); } } - this.loadSample(sample, arrayBuffers); + this.loadSample(sample, arrayBuffers.flat()); } getSample(): Promise { @@ -741,21 +759,31 @@ export abstract class AbstractLooker< labelsWorker.addEventListener("message", listener); - labelsWorker.postMessage( - { - sample: sample as ProcessSample["sample"], - method: "processSample", - coloring: this.state.options.coloring, - customizeColorSetting: this.state.options.customizeColorSetting, - colorscale: this.state.options.colorscale, - labelTagColors: this.state.options.labelTagColors, - selectedLabelTags: this.state.options.selectedLabelTags, - sources: this.state.config.sources, - schema: this.state.config.fieldSchema, - uuid: messageUUID, - } as ProcessSample, - transfer - ); + const workerArgs = { + sample: sample as ProcessSample["sample"], + method: "processSample", + coloring: this.state.options.coloring, + customizeColorSetting: this.state.options.customizeColorSetting, + colorscale: this.state.options.colorscale, + labelTagColors: this.state.options.labelTagColors, + selectedLabelTags: this.state.options.selectedLabelTags, + sources: this.state.config.sources, + schema: this.state.config.fieldSchema, + uuid: messageUUID, + } as ProcessSample; + + try { + labelsWorker.postMessage(workerArgs, transfer); + } catch (error) { + // rarely we'll get a DataCloneError + // if one of the buffers is detached and we didn't catch it + // try again without transferring the buffers (copying them) + if (error.name === "DataCloneError") { + labelsWorker.postMessage(workerArgs); + } else { + throw error; + } + } } } diff --git a/app/packages/looker/src/worker/index.ts b/app/packages/looker/src/worker/index.ts index dcf0b2e79b..3f1e6cefeb 100644 --- a/app/packages/looker/src/worker/index.ts +++ b/app/packages/looker/src/worker/index.ts @@ -327,6 +327,10 @@ const processSample = async ({ labelTagColors, schema, }: ProcessSample) => { + if (!sample) { + return; + } + mapId(sample); const imageBitmapPromises: Promise[] = []; From d3bf17472338ec0838f03800dee5f22cac12736b Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Tue, 10 Dec 2024 10:48:39 -0600 Subject: [PATCH 4/7] add clarifying comments --- app/packages/looker/src/lookers/abstract.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/packages/looker/src/lookers/abstract.ts b/app/packages/looker/src/lookers/abstract.ts index 6234ae6082..499e2634a5 100644 --- a/app/packages/looker/src/lookers/abstract.ts +++ b/app/packages/looker/src/lookers/abstract.ts @@ -547,9 +547,10 @@ export abstract class AbstractLooker< } else { arrayBuffers.push(buffer); } - } else { + } else if (buffer.byteLength) { // hope we don't run into this edge case (old browser) - // if we do, we'll just copy the buffer + // sometimes detached buffers have bytelength > 0 + // if we run into this case, we'll just attempt to transfer the buffer // might get a DataCloneError if user is switching colors too fast arrayBuffers.push(buffer); } From 4f76488631edecb8dc8b210ff4a59646bfdb5c06 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Thu, 12 Dec 2024 15:55:43 -0600 Subject: [PATCH 5/7] cleanup overlays in the worker listener callback instead --- app/packages/looker/src/lookers/abstract.ts | 13 +++++++++---- app/packages/looker/src/overlays/base.ts | 1 - app/packages/looker/src/overlays/detection.ts | 12 ++---------- app/packages/looker/src/overlays/heatmap.ts | 12 ++---------- app/packages/looker/src/overlays/segmentation.ts | 12 ++---------- .../looker/src/worker/disk-overlay-decoder.ts | 11 ++++++++--- 6 files changed, 23 insertions(+), 38 deletions(-) diff --git a/app/packages/looker/src/lookers/abstract.ts b/app/packages/looker/src/lookers/abstract.ts index 499e2634a5..1043c4d7b8 100644 --- a/app/packages/looker/src/lookers/abstract.ts +++ b/app/packages/looker/src/lookers/abstract.ts @@ -520,10 +520,6 @@ export abstract class AbstractLooker< const arrayBuffers: ArrayBuffer[] = []; for (const overlay of this.pluckedOverlays ?? []) { - // we paint overlays again, so cleanup the old ones - // this helps prevent memory leaks from, for instance, dangling ImageBitmaps - overlay.cleanup(); - let overlayData: LabelMask = null; if ("mask" in overlay.label) { @@ -739,6 +735,12 @@ export abstract class AbstractLooker< ); } + protected cleanOverlays() { + for (const overlay of this.sampleOverlays ?? []) { + overlay.cleanup(); + } + } + private loadSample(sample: Sample, transfer: Transferable[] = []) { const messageUUID = uuid(); @@ -746,6 +748,9 @@ export abstract class AbstractLooker< const listener = ({ data: { sample, coloring, uuid } }) => { if (uuid === messageUUID) { + // we paint overlays again, so cleanup the old ones + // this helps prevent memory leaks from, for instance, dangling ImageBitmaps + this.cleanOverlays(); this.sample = sample; this.state.options.coloring = coloring; this.loadOverlays(sample); diff --git a/app/packages/looker/src/overlays/base.ts b/app/packages/looker/src/overlays/base.ts index 9b433e9400..faf6f284b5 100644 --- a/app/packages/looker/src/overlays/base.ts +++ b/app/packages/looker/src/overlays/base.ts @@ -42,7 +42,6 @@ export interface SelectData { export type LabelMask = { bitmap?: ImageBitmap; - closedBitmapDims?: { width: number; height: number }; data?: OverlayMask; }; diff --git a/app/packages/looker/src/overlays/detection.ts b/app/packages/looker/src/overlays/detection.ts index d5b80d9873..1298cf2fbc 100644 --- a/app/packages/looker/src/overlays/detection.ts +++ b/app/packages/looker/src/overlays/detection.ts @@ -262,16 +262,8 @@ export default class DetectionOverlay< } public cleanup(): void { - if (this.label.mask?.bitmap) { - // store height and width in bitmap object since it might be used again - const height = this.label.mask.bitmap.height; - const width = this.label.mask.bitmap.width; - - this.label.mask?.bitmap.close(); - this.label.mask.bitmap = null; - - this.label.mask.closedBitmapDims = { width, height }; - } + this.label.mask?.bitmap?.close(); + this.label.mask.bitmap = null; } } diff --git a/app/packages/looker/src/overlays/heatmap.ts b/app/packages/looker/src/overlays/heatmap.ts index c1be209ed5..b9e41e1a9c 100644 --- a/app/packages/looker/src/overlays/heatmap.ts +++ b/app/packages/looker/src/overlays/heatmap.ts @@ -207,16 +207,8 @@ export default class HeatmapOverlay } public cleanup(): void { - if (this.label.map?.bitmap) { - // store height and width in bitmap object since it might be used again - const height = this.label.map.bitmap.height; - const width = this.label.map.bitmap.width; - - this.label.map?.bitmap.close(); - this.label.map.bitmap = null; - - this.label.map.closedBitmapDims = { width, height }; - } + this.label.map?.bitmap?.close(); + this.label.map.bitmap = null; } } diff --git a/app/packages/looker/src/overlays/segmentation.ts b/app/packages/looker/src/overlays/segmentation.ts index 3218db80a6..566a7153b0 100644 --- a/app/packages/looker/src/overlays/segmentation.ts +++ b/app/packages/looker/src/overlays/segmentation.ts @@ -262,16 +262,8 @@ export default class SegmentationOverlay } public cleanup(): void { - if (this.label.mask?.bitmap) { - // store height and width in bitmap object since it might be used again - const height = this.label.mask.bitmap.height; - const width = this.label.mask.bitmap.width; - - this.label.mask?.bitmap.close(); - this.label.mask.bitmap = null; - - this.label.mask.closedBitmapDims = { width, height }; - } + this.label.mask?.bitmap?.close(); + this.label.mask.bitmap = null; } } diff --git a/app/packages/looker/src/worker/disk-overlay-decoder.ts b/app/packages/looker/src/worker/disk-overlay-decoder.ts index e2cbf6081b..c5ac65acd1 100644 --- a/app/packages/looker/src/worker/disk-overlay-decoder.ts +++ b/app/packages/looker/src/worker/disk-overlay-decoder.ts @@ -56,11 +56,16 @@ export const decodeOverlayOnDisk = async ( // it's possible we're just re-coloring, in which case re-init mask image and set bitmap to null if ( label[overlayField] && - label[overlayField].closedBitmapDims && + label[overlayField].bitmap && !label[overlayField].image ) { - const height = label[overlayField].closedBitmapDims.height; - const width = label[overlayField].closedBitmapDims.width; + const height = label[overlayField].bitmap.height; + const width = label[overlayField].bitmap.width; + + // close the copied bitmap + label[overlayField].bitmap.close(); + label[overlayField].bitmap = null; + label[overlayField].image = new ArrayBuffer(height * width * 4); label[overlayField].bitmap.close(); label[overlayField].bitmap = null; From 1510b47fccab0aac3512026982559fc5ecca63d8 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Thu, 12 Dec 2024 16:43:42 -0600 Subject: [PATCH 6/7] remove bitmap = null (obj closed for modification) --- app/packages/looker/src/overlays/detection.ts | 1 - app/packages/looker/src/overlays/heatmap.ts | 1 - app/packages/looker/src/overlays/segmentation.ts | 1 - 3 files changed, 3 deletions(-) diff --git a/app/packages/looker/src/overlays/detection.ts b/app/packages/looker/src/overlays/detection.ts index 1298cf2fbc..e8616f71b1 100644 --- a/app/packages/looker/src/overlays/detection.ts +++ b/app/packages/looker/src/overlays/detection.ts @@ -263,7 +263,6 @@ export default class DetectionOverlay< public cleanup(): void { this.label.mask?.bitmap?.close(); - this.label.mask.bitmap = null; } } diff --git a/app/packages/looker/src/overlays/heatmap.ts b/app/packages/looker/src/overlays/heatmap.ts index b9e41e1a9c..d8fb8909d5 100644 --- a/app/packages/looker/src/overlays/heatmap.ts +++ b/app/packages/looker/src/overlays/heatmap.ts @@ -208,7 +208,6 @@ export default class HeatmapOverlay public cleanup(): void { this.label.map?.bitmap?.close(); - this.label.map.bitmap = null; } } diff --git a/app/packages/looker/src/overlays/segmentation.ts b/app/packages/looker/src/overlays/segmentation.ts index 566a7153b0..04c2fc693b 100644 --- a/app/packages/looker/src/overlays/segmentation.ts +++ b/app/packages/looker/src/overlays/segmentation.ts @@ -263,7 +263,6 @@ export default class SegmentationOverlay public cleanup(): void { this.label.mask?.bitmap?.close(); - this.label.mask.bitmap = null; } } From e22b56ec942eee32064a49ac1ac74fa734fe36f5 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Thu, 12 Dec 2024 16:50:55 -0600 Subject: [PATCH 7/7] remove unnecessary sample null check guard --- app/packages/looker/src/worker/index.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/packages/looker/src/worker/index.ts b/app/packages/looker/src/worker/index.ts index 3f1e6cefeb..dcf0b2e79b 100644 --- a/app/packages/looker/src/worker/index.ts +++ b/app/packages/looker/src/worker/index.ts @@ -327,10 +327,6 @@ const processSample = async ({ labelTagColors, schema, }: ProcessSample) => { - if (!sample) { - return; - } - mapId(sample); const imageBitmapPromises: Promise[] = [];