From 0727741ca1f061ca7d3fd8b94c6f290872c6d236 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Sat, 27 Aug 2022 20:50:16 -0700 Subject: [PATCH] Remove iterators from IDecorationService They're too low performance for typical use cases Fixes #4079 --- addons/xterm-addon-canvas/src/BaseRenderLayer.ts | 11 +++++------ addons/xterm-addon-canvas/src/TextRenderLayer.ts | 6 +++--- src/browser/renderer/dom/DomRendererRowFactory.ts | 6 +++--- src/common/TestUtils.test.ts | 2 -- src/common/services/DecorationService.ts | 4 ---- src/common/services/Services.ts | 8 ++------ 6 files changed, 13 insertions(+), 24 deletions(-) diff --git a/addons/xterm-addon-canvas/src/BaseRenderLayer.ts b/addons/xterm-addon-canvas/src/BaseRenderLayer.ts index dfd814e3..ae396016 100644 --- a/addons/xterm-addon-canvas/src/BaseRenderLayer.ts +++ b/addons/xterm-addon-canvas/src/BaseRenderLayer.ts @@ -404,12 +404,11 @@ export abstract class BaseRenderLayer implements IRenderLayer { // Don't try cache the glyph if it uses any decoration foreground/background override. let hasOverrides = false; - for (const d of this._decorationService.getDecorationsAtCell(x, y)) { + this._decorationService.forEachDecorationAtCell(x, y, undefined, d => { if (d.backgroundColorRGB || d.foregroundColorRGB) { hasOverrides = true; - break; } - } + }); const atlasDidDraw = hasOverrides ? false : this._charAtlas?.draw(this._ctx, this._currentGlyphIdentifier, x * this._scaledCellWidth + this._scaledCharLeft, y * this._scaledCellHeight + this._scaledCharTop); @@ -519,9 +518,9 @@ export abstract class BaseRenderLayer implements IRenderLayer { let bgOverride: number | undefined; let fgOverride: number | undefined; let isTop = false; - for (const d of this._decorationService.getDecorationsAtCell(x, y)) { + this._decorationService.forEachDecorationAtCell(x, y, undefined, d => { if (d.options.layer !== 'top' && isTop) { - continue; + return; } if (d.backgroundColorRGB) { bgOverride = d.backgroundColorRGB.rgba; @@ -530,7 +529,7 @@ export abstract class BaseRenderLayer implements IRenderLayer { fgOverride = d.foregroundColorRGB.rgba; } isTop = d.options.layer === 'top'; - } + }); // Apply selection foreground if applicable if (!isTop) { diff --git a/addons/xterm-addon-canvas/src/TextRenderLayer.ts b/addons/xterm-addon-canvas/src/TextRenderLayer.ts index 0308f125..86e21ad5 100644 --- a/addons/xterm-addon-canvas/src/TextRenderLayer.ts +++ b/addons/xterm-addon-canvas/src/TextRenderLayer.ts @@ -187,15 +187,15 @@ export class TextRenderLayer extends BaseRenderLayer { // Get any decoration foreground/background overrides, this must be fetched before the early // exist but applied after inverse let isTop = false; - for (const d of this._decorationService.getDecorationsAtCell(x, this._bufferService.buffer.ydisp + y)) { + this._decorationService.forEachDecorationAtCell(x, this._bufferService.buffer.ydisp + y, undefined, d => { if (d.options.layer !== 'top' && isTop) { - continue; + return; } if (d.backgroundColorRGB) { nextFillStyle = d.backgroundColorRGB.css; } isTop = d.options.layer === 'top'; - } + }); if (prevFillStyle === null) { // This is either the first iteration, or the default background was set. Either way, we diff --git a/src/browser/renderer/dom/DomRendererRowFactory.ts b/src/browser/renderer/dom/DomRendererRowFactory.ts index cf4b3680..d3eb9e8e 100644 --- a/src/browser/renderer/dom/DomRendererRowFactory.ts +++ b/src/browser/renderer/dom/DomRendererRowFactory.ts @@ -204,9 +204,9 @@ export class DomRendererRowFactory { let bgOverride: IColor | undefined; let fgOverride: IColor | undefined; let isTop = false; - for (const d of this._decorationService.getDecorationsAtCell(x, row)) { + this._decorationService.forEachDecorationAtCell(x, row, undefined, d => { if (d.options.layer !== 'top' && isTop) { - continue; + return; } if (d.backgroundColorRGB) { bgColorMode = Attributes.CM_RGB; @@ -219,7 +219,7 @@ export class DomRendererRowFactory { fgOverride = d.foregroundColorRGB; } isTop = d.options.layer === 'top'; - } + }); // Apply selection foreground if applicable const isInSelection = this._isCellInSelection(x, row); diff --git a/src/common/TestUtils.test.ts b/src/common/TestUtils.test.ts index cbcdd36e..ff1e1b46 100644 --- a/src/common/TestUtils.test.ts +++ b/src/common/TestUtils.test.ts @@ -161,8 +161,6 @@ export class MockDecorationService implements IDecorationService { public onDecorationRemoved = new EventEmitter().event; public registerDecoration(decorationOptions: IDecorationOptions): IDecoration | undefined { return undefined; } public reset(): void { } - public *getDecorationsAtLine(line: number): IterableIterator { } - public *getDecorationsAtCell(x: number, line: number): IterableIterator { } public forEachDecorationAtCell(x: number, line: number, layer: 'bottom' | 'top' | undefined, callback: (decoration: IInternalDecoration) => void): void { } public dispose(): void { } } diff --git a/src/common/services/DecorationService.ts b/src/common/services/DecorationService.ts index 89571363..e5d115a1 100644 --- a/src/common/services/DecorationService.ts +++ b/src/common/services/DecorationService.ts @@ -62,10 +62,6 @@ export class DecorationService extends Disposable implements IDecorationService this._decorations.clear(); } - public *getDecorationsAtLine(line: number): IterableIterator { - return this._decorations.getKeyIterator(line); - } - public *getDecorationsAtCell(x: number, line: number, layer?: 'bottom' | 'top'): IterableIterator { let xmin = 0; let xmax = 0; diff --git a/src/common/services/Services.ts b/src/common/services/Services.ts index b8e227b3..817a7680 100644 --- a/src/common/services/Services.ts +++ b/src/common/services/Services.ts @@ -304,13 +304,9 @@ export interface IDecorationService extends IDisposable { readonly onDecorationRemoved: IEvent; registerDecoration(decorationOptions: IDecorationOptions): IDecoration | undefined; reset(): void; - /** Iterates over the decorations at a line (in no particular order). */ - getDecorationsAtLine(line: number): IterableIterator; - /** Iterates over the decorations at a cell (in no particular order). */ - getDecorationsAtCell(x: number, line: number, layer?: 'bottom' | 'top'): IterableIterator; /** - * Trigger a callback over the decoration at a cell (in no particular order). This is a high - * performance, but less ergonomic, version of {@link getDecorationsAtCell}. + * Trigger a callback over the decoration at a cell (in no particular order). This uses a callback + * instead of an iterator as it's typically used in hot code paths. */ forEachDecorationAtCell(x: number, line: number, layer: 'bottom' | 'top' | undefined, callback: (decoration: IInternalDecoration) => void): void; }