From 5e3f63e876dcf875b7509c951c01818699a42370 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Sat, 27 Aug 2022 20:06:58 -0700 Subject: [PATCH 1/3] Avoid garbage collection in webgl decoration hot code Part of #4079 --- addons/xterm-addon-webgl/src/WebglRenderer.ts | 81 ++++++++++++------- src/common/SortedList.ts | 25 +++++- src/common/TestUtils.test.ts | 1 + src/common/services/DecorationService.ts | 16 ++++ src/common/services/Services.ts | 5 ++ 5 files changed, 94 insertions(+), 34 deletions(-) diff --git a/addons/xterm-addon-webgl/src/WebglRenderer.ts b/addons/xterm-addon-webgl/src/WebglRenderer.ts index 9e90df02..efb80f66 100644 --- a/addons/xterm-addon-webgl/src/WebglRenderer.ts +++ b/addons/xterm-addon-webgl/src/WebglRenderer.ts @@ -11,7 +11,7 @@ import { WebglCharAtlas } from './atlas/WebglCharAtlas'; import { RectangleRenderer } from './RectangleRenderer'; import { IWebGL2RenderingContext } from './Types'; import { RenderModel, COMBINED_CHAR_BIT_MASK, RENDER_MODEL_BG_OFFSET, RENDER_MODEL_FG_OFFSET, RENDER_MODEL_EXT_OFFSET, RENDER_MODEL_INDICIES_PER_CELL } from './RenderModel'; -import { Disposable, toDisposable } from 'common/Lifecycle'; +import { Disposable } from 'common/Lifecycle'; import { Attributes, BgFlags, Content, FgFlags, NULL_CELL_CHAR, NULL_CELL_CODE } from 'common/buffer/Constants'; import { Terminal, IEvent } from 'xterm'; import { IRenderLayer } from './renderLayer/Types'; @@ -26,6 +26,15 @@ import { CharData, ICellData } from 'common/Types'; import { AttributeData } from 'common/buffer/AttributeData'; import { ICoreService, IDecorationService } from 'common/services/Services'; +/** Work variables to avoid garbage collection. */ +const w: { fg: number, bg: number, hasFg: boolean, hasBg: boolean, isSelected: boolean } = { + fg: 0, + bg: 0, + hasFg: false, + hasBg: false, + isSelected: false +}; + export class WebglRenderer extends Disposable implements IRenderer { private _renderLayers: IRenderLayer[]; private _charAtlas: WebglCharAtlas | undefined; @@ -404,79 +413,89 @@ export class WebglRenderer extends Disposable implements IRenderer { this._workColors.ext = this._workCell.bg & BgFlags.HAS_EXTENDED ? this._workCell.extended.ext : 0; // Get any foreground/background overrides, this happens on the model to avoid spreading // override logic throughout the different sub-renderers - let bgOverride: number | undefined; - let fgOverride: number | undefined; - let isSelected: boolean = false; + + // Reset overrides work variables + w.bg = 0; + w.fg = 0; + w.hasBg = false; + w.hasFg = false; + w.isSelected = false; // Apply decorations on the bottom layer - for (const d of this._decorationService.getDecorationsAtCell(x, y, 'bottom')) { + this._decorationService.forEachDecorationAtCell(x, y, 'bottom', d => { if (d.backgroundColorRGB) { - bgOverride = d.backgroundColorRGB.rgba >> 8 & 0xFFFFFF; + w.bg = d.backgroundColorRGB.rgba >> 8 & 0xFFFFFF; + w.hasBg = true; } if (d.foregroundColorRGB) { - fgOverride = d.foregroundColorRGB.rgba >> 8 & 0xFFFFFF; + w.fg = d.foregroundColorRGB.rgba >> 8 & 0xFFFFFF; + w.hasFg = true; } - } + }); // Apply the selection color if needed - isSelected = this._isCellSelected(x, y); - if (isSelected) { - bgOverride = (this._coreBrowserService.isFocused ? this._colors.selectionBackgroundOpaque : this._colors.selectionInactiveBackgroundOpaque).rgba >> 8 & 0xFFFFFF; + w.isSelected = this._isCellSelected(x, y); + if (w.isSelected) { + w.bg = (this._coreBrowserService.isFocused ? this._colors.selectionBackgroundOpaque : this._colors.selectionInactiveBackgroundOpaque).rgba >> 8 & 0xFFFFFF; + w.hasBg = true; if (this._colors.selectionForeground) { - fgOverride = this._colors.selectionForeground.rgba >> 8 & 0xFFFFFF; + w.fg = this._colors.selectionForeground.rgba >> 8 & 0xFFFFFF; + w.hasFg = true; } } // Apply decorations on the top layer - for (const d of this._decorationService.getDecorationsAtCell(x, y, 'top')) { + this._decorationService.forEachDecorationAtCell(x, y, 'top', d => { if (d.backgroundColorRGB) { - bgOverride = d.backgroundColorRGB.rgba >> 8 & 0xFFFFFF; + w.bg = d.backgroundColorRGB.rgba >> 8 & 0xFFFFFF; + w.hasBg = true; } if (d.foregroundColorRGB) { - fgOverride = d.foregroundColorRGB.rgba >> 8 & 0xFFFFFF; + w.fg = d.foregroundColorRGB.rgba >> 8 & 0xFFFFFF; + w.hasFg = true; } - } + }); // Convert any overrides from rgba to the fg/bg packed format. This resolves the inverse flag // ahead of time in order to use the correct cache key - if (bgOverride !== undefined) { - if (isSelected) { + if (w.hasBg) { + if (w.isSelected) { // Non-RGB attributes from model + force non-dim + override + force RGB color mode - bgOverride = (this._workCell.bg & ~Attributes.RGB_MASK & ~BgFlags.DIM) | bgOverride | Attributes.CM_RGB; + w.bg = (this._workCell.bg & ~Attributes.RGB_MASK & ~BgFlags.DIM) | w.bg | Attributes.CM_RGB; } else { // Non-RGB attributes from model + override + force RGB color mode - bgOverride = (this._workCell.bg & ~Attributes.RGB_MASK) | bgOverride | Attributes.CM_RGB; + w.bg = (this._workCell.bg & ~Attributes.RGB_MASK) | w.bg | Attributes.CM_RGB; } } - if (fgOverride !== undefined) { + if (w.hasFg) { // Non-RGB attributes from model + force disable inverse + override + force RGB color mode - fgOverride = (this._workCell.fg & ~Attributes.RGB_MASK & ~FgFlags.INVERSE) | fgOverride | Attributes.CM_RGB; + w.fg = (this._workCell.fg & ~Attributes.RGB_MASK & ~FgFlags.INVERSE) | w.fg | Attributes.CM_RGB; } - // Handle case where inverse was specified by only one of bgOverride or fgOverride was set, + // Handle case where inverse was specified by only one of bg override or fg override was set, // resolving the other inverse color and setting the inverse flag if needed. if (this._workColors.fg & FgFlags.INVERSE) { - if (bgOverride !== undefined && fgOverride === undefined) { + if (w.hasBg && w.hasFg) { // Resolve bg color type (default color has a different meaning in fg vs bg) if ((this._workColors.bg & Attributes.CM_MASK) === Attributes.CM_DEFAULT) { - fgOverride = (this._workColors.fg & ~(Attributes.RGB_MASK | FgFlags.INVERSE | Attributes.CM_MASK)) | ((this._colors.background.rgba >> 8 & 0xFFFFFF) & Attributes.RGB_MASK) | Attributes.CM_RGB; + w.fg = (this._workColors.fg & ~(Attributes.RGB_MASK | FgFlags.INVERSE | Attributes.CM_MASK)) | ((this._colors.background.rgba >> 8 & 0xFFFFFF) & Attributes.RGB_MASK) | Attributes.CM_RGB; } else { - fgOverride = (this._workColors.fg & ~(Attributes.RGB_MASK | FgFlags.INVERSE | Attributes.CM_MASK)) | this._workColors.bg & (Attributes.RGB_MASK | Attributes.CM_MASK); + w.fg = (this._workColors.fg & ~(Attributes.RGB_MASK | FgFlags.INVERSE | Attributes.CM_MASK)) | this._workColors.bg & (Attributes.RGB_MASK | Attributes.CM_MASK); } } - if (bgOverride === undefined && fgOverride !== undefined) { + if (w.hasBg && w.hasFg) { // Resolve bg color type (default color has a different meaning in fg vs bg) if ((this._workColors.fg & Attributes.CM_MASK) === Attributes.CM_DEFAULT) { - bgOverride = (this._workColors.bg & ~(Attributes.RGB_MASK | Attributes.CM_MASK)) | ((this._colors.foreground.rgba >> 8 & 0xFFFFFF) & Attributes.RGB_MASK) | Attributes.CM_RGB; + w.bg = (this._workColors.bg & ~(Attributes.RGB_MASK | Attributes.CM_MASK)) | ((this._colors.foreground.rgba >> 8 & 0xFFFFFF) & Attributes.RGB_MASK) | Attributes.CM_RGB; } else { - bgOverride = (this._workColors.bg & ~(Attributes.RGB_MASK | Attributes.CM_MASK)) | this._workColors.fg & (Attributes.RGB_MASK | Attributes.CM_MASK); + w.bg = (this._workColors.bg & ~(Attributes.RGB_MASK | Attributes.CM_MASK)) | this._workColors.fg & (Attributes.RGB_MASK | Attributes.CM_MASK); } } } // Use the override if it exists - this._workColors.bg = bgOverride ?? this._workColors.bg; - this._workColors.fg = fgOverride ?? this._workColors.fg; + this._workColors.bg = w.bg ?? this._workColors.bg; + this._workColors.fg = w.fg ?? this._workColors.fg; } private _isCellSelected(x: number, y: number): boolean { diff --git a/src/common/SortedList.ts b/src/common/SortedList.ts index 9c819959..737b9acb 100644 --- a/src/common/SortedList.ts +++ b/src/common/SortedList.ts @@ -3,6 +3,9 @@ * @license MIT */ +// Work variables to avoid garbage collection. +let i = 0; + /** * A generic list that is maintained in sorted order and allows values with duplicate keys. This * list is based on binary search and as such locating a key will take O(log n) amortized, this @@ -25,7 +28,7 @@ export class SortedList { this._array.push(value); return; } - const i = this._search(this._getKey(value), 0, this._array.length - 1); + i = this._search(this._getKey(value), 0, this._array.length - 1); this._array.splice(i, 0, value); } @@ -37,7 +40,7 @@ export class SortedList { if (key === undefined) { return false; } - let i = this._search(key, 0, this._array.length - 1); + i = this._search(key, 0, this._array.length - 1); if (i === -1) { return false; } @@ -57,7 +60,7 @@ export class SortedList { if (this._array.length === 0) { return; } - let i = this._search(key, 0, this._array.length - 1); + i = this._search(key, 0, this._array.length - 1); if (i < 0 || i >= this._array.length) { return; } @@ -69,6 +72,22 @@ export class SortedList { } while (++i < this._array.length && this._getKey(this._array[i]) === key); } + public forEachByKey(key: number, callback: (value: T) => void): void { + if (this._array.length === 0) { + return; + } + i = this._search(key, 0, this._array.length - 1); + if (i < 0 || i >= this._array.length) { + return; + } + if (this._getKey(this._array[i]) !== key) { + return; + } + do { + callback(this._array[i]); + } while (++i < this._array.length && this._getKey(this._array[i]) === key); + } + public values(): IterableIterator { return this._array.values(); } diff --git a/src/common/TestUtils.test.ts b/src/common/TestUtils.test.ts index 48f3a69e..cbcdd36e 100644 --- a/src/common/TestUtils.test.ts +++ b/src/common/TestUtils.test.ts @@ -163,5 +163,6 @@ export class MockDecorationService implements IDecorationService { 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 58bff729..89571363 100644 --- a/src/common/services/DecorationService.ts +++ b/src/common/services/DecorationService.ts @@ -11,6 +11,12 @@ import { SortedList } from 'common/SortedList'; import { IColor } from 'common/Types'; import { IDecorationOptions, IDecoration, IMarker, IEvent } from 'xterm'; +/** Work variables to avoid garbage collection. */ +const w = { + xmin: 0, + xmax: 0 +}; + export class DecorationService extends Disposable implements IDecorationService { public serviceBrand: any; @@ -72,6 +78,16 @@ export class DecorationService extends Disposable implements IDecorationService } } + public forEachDecorationAtCell(x: number, line: number, layer: 'bottom' | 'top' | undefined, callback: (decoration: IInternalDecoration) => void): void { + this._decorations.forEachByKey(line, d => { + w.xmin = d.options.x ?? 0; + w.xmax = w.xmin + (d.options.width ?? 1); + if (x >= w.xmin && x < w.xmax && (!layer || (d.options.layer ?? 'bottom') === layer)) { + callback(d); + } + }); + } + public dispose(): void { for (const d of this._decorations.values()) { this._onDecorationRemoved.fire(d); diff --git a/src/common/services/Services.ts b/src/common/services/Services.ts index 585b29ac..b8e227b3 100644 --- a/src/common/services/Services.ts +++ b/src/common/services/Services.ts @@ -308,6 +308,11 @@ export interface IDecorationService extends IDisposable { 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}. + */ + forEachDecorationAtCell(x: number, line: number, layer: 'bottom' | 'top' | undefined, callback: (decoration: IInternalDecoration) => void): void; } export interface IInternalDecoration extends IDecoration { readonly options: IDecorationOptions; 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 2/3] 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; } From 1b64a09f50bcc1edda5dc16e2ff7fcd3de7602d5 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Sat, 27 Aug 2022 21:35:56 -0700 Subject: [PATCH 3/3] Correct conditions after moving to work object --- addons/xterm-addon-webgl/src/WebglRenderer.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/addons/xterm-addon-webgl/src/WebglRenderer.ts b/addons/xterm-addon-webgl/src/WebglRenderer.ts index efb80f66..ea10b368 100644 --- a/addons/xterm-addon-webgl/src/WebglRenderer.ts +++ b/addons/xterm-addon-webgl/src/WebglRenderer.ts @@ -475,27 +475,29 @@ export class WebglRenderer extends Disposable implements IRenderer { // Handle case where inverse was specified by only one of bg override or fg override was set, // resolving the other inverse color and setting the inverse flag if needed. if (this._workColors.fg & FgFlags.INVERSE) { - if (w.hasBg && w.hasFg) { + if (w.hasBg && !w.hasFg) { // Resolve bg color type (default color has a different meaning in fg vs bg) if ((this._workColors.bg & Attributes.CM_MASK) === Attributes.CM_DEFAULT) { w.fg = (this._workColors.fg & ~(Attributes.RGB_MASK | FgFlags.INVERSE | Attributes.CM_MASK)) | ((this._colors.background.rgba >> 8 & 0xFFFFFF) & Attributes.RGB_MASK) | Attributes.CM_RGB; } else { w.fg = (this._workColors.fg & ~(Attributes.RGB_MASK | FgFlags.INVERSE | Attributes.CM_MASK)) | this._workColors.bg & (Attributes.RGB_MASK | Attributes.CM_MASK); } + w.hasFg = true; } - if (w.hasBg && w.hasFg) { + if (!w.hasBg && w.hasFg) { // Resolve bg color type (default color has a different meaning in fg vs bg) if ((this._workColors.fg & Attributes.CM_MASK) === Attributes.CM_DEFAULT) { w.bg = (this._workColors.bg & ~(Attributes.RGB_MASK | Attributes.CM_MASK)) | ((this._colors.foreground.rgba >> 8 & 0xFFFFFF) & Attributes.RGB_MASK) | Attributes.CM_RGB; } else { w.bg = (this._workColors.bg & ~(Attributes.RGB_MASK | Attributes.CM_MASK)) | this._workColors.fg & (Attributes.RGB_MASK | Attributes.CM_MASK); } + w.hasBg = true; } } // Use the override if it exists - this._workColors.bg = w.bg ?? this._workColors.bg; - this._workColors.fg = w.fg ?? this._workColors.fg; + this._workColors.bg = w.hasBg ? w.bg : this._workColors.bg; + this._workColors.fg = w.hasFg ? w.fg : this._workColors.fg; } private _isCellSelected(x: number, y: number): boolean {