From 04f2d52f26342dc9a88ba65d28e889a43f38ab39 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 14 Sep 2018 09:58:49 -0700 Subject: [PATCH 01/26] Remove IGlyphIdentifier in-between object This was being constructed for every character draw --- src/renderer/BaseRenderLayer.ts | 8 +++- src/renderer/atlas/BaseCharAtlas.ts | 19 ++++++-- src/renderer/atlas/DynamicCharAtlas.ts | 64 ++++++++++++++++---------- src/renderer/atlas/NoneCharAtlas.ts | 9 +++- src/renderer/atlas/StaticCharAtlas.ts | 34 ++++++++------ src/renderer/atlas/Types.ts | 10 ---- 6 files changed, 88 insertions(+), 56 deletions(-) diff --git a/src/renderer/BaseRenderLayer.ts b/src/renderer/BaseRenderLayer.ts index 1df9c3ea..d7755aca 100644 --- a/src/renderer/BaseRenderLayer.ts +++ b/src/renderer/BaseRenderLayer.ts @@ -247,7 +247,13 @@ export abstract class BaseRenderLayer implements IRenderLayer { fg += drawInBrightColor ? 8 : 0; const atlasDidDraw = this._charAtlas && this._charAtlas.draw( this._ctx, - {chars, code, bg, fg, bold: bold && terminal.options.enableBold, dim, italic}, + chars, + code, + bg, + fg, + bold, + dim, + italic, x * this._scaledCellWidth + this._scaledCharLeft, y * this._scaledCellHeight + this._scaledCharTop ); diff --git a/src/renderer/atlas/BaseCharAtlas.ts b/src/renderer/atlas/BaseCharAtlas.ts index 50d35faa..325818c6 100644 --- a/src/renderer/atlas/BaseCharAtlas.ts +++ b/src/renderer/atlas/BaseCharAtlas.ts @@ -3,8 +3,6 @@ * @license MIT */ -import { IGlyphIdentifier } from './Types'; - export default abstract class BaseCharAtlas { private _didWarmUp: boolean = false; @@ -39,14 +37,27 @@ export default abstract class BaseCharAtlas { * do nothing and return false in that case. * * @param ctx Where to draw the character onto. - * @param glyph Information about what to draw + * @param chars The character(s) to draw. This is typically a single character bug can be made up + * of multiple when character joiners are used. + * @param code The character code. + * @param bg The background color. + * @param fg The foreground color. + * @param bold Whether the text is bold. + * @param dim Whether the text is dim. + * @param italic Whether the text is italic. * @param x The position on the context to start drawing at * @param y The position on the context to start drawing at * @returns The success state. True if we drew the character. */ public abstract draw( ctx: CanvasRenderingContext2D, - glyph: IGlyphIdentifier, + chars: string, + code: number, + bg: number, + fg: number, + bold: boolean, + dim: boolean, + italic: boolean, x: number, y: number ): boolean; diff --git a/src/renderer/atlas/DynamicCharAtlas.ts b/src/renderer/atlas/DynamicCharAtlas.ts index f6df365e..66baa822 100644 --- a/src/renderer/atlas/DynamicCharAtlas.ts +++ b/src/renderer/atlas/DynamicCharAtlas.ts @@ -3,7 +3,7 @@ * @license MIT */ -import { DIM_OPACITY, IGlyphIdentifier, INVERTED_DEFAULT_COLOR } from './Types'; +import { DIM_OPACITY, INVERTED_DEFAULT_COLOR } from './Types'; import { ICharAtlasConfig } from '../../shared/atlas/Types'; import { IColor } from '../../shared/Types'; import BaseCharAtlas from './BaseCharAtlas'; @@ -34,9 +34,9 @@ interface IGlyphCacheValue { isEmpty: boolean; } -function getGlyphCacheKey(glyph: IGlyphIdentifier): string { - const styleFlags = (glyph.bold ? 0 : 4) + (glyph.dim ? 0 : 2) + (glyph.italic ? 0 : 1); - return `${glyph.bg}_${glyph.fg}_${styleFlags}${glyph.chars}`; +function getGlyphCacheKey(chars: string, fg: number, bg: number, bold: boolean, dim: boolean, italic: boolean): string { + const styleFlags = (bold ? 0 : 4) + (dim ? 0 : 2) + (italic ? 0 : 1); + return `${bg}_${fg}_${styleFlags}${chars}`; } export default class DynamicCharAtlas extends BaseCharAtlas { @@ -88,16 +88,22 @@ export default class DynamicCharAtlas extends BaseCharAtlas { public draw( ctx: CanvasRenderingContext2D, - glyph: IGlyphIdentifier, + chars: string, + code: number, + bg: number, + fg: number, + bold: boolean, + dim: boolean, + italic: boolean, x: number, y: number ): boolean { - const glyphKey = getGlyphCacheKey(glyph); + const glyphKey = getGlyphCacheKey(chars, fg, bg, bold, dim, italic); const cacheValue = this._cacheMap.get(glyphKey); if (cacheValue !== null && cacheValue !== undefined) { this._drawFromCache(ctx, cacheValue, x, y); return true; - } else if (this._canCache(glyph) && this._drawToCacheCount < FRAME_CACHE_DRAW_LIMIT) { + } else if (this._canCache(code) && this._drawToCacheCount < FRAME_CACHE_DRAW_LIMIT) { let index; if (this._cacheMap.size < this._cacheMap.capacity) { index = this._cacheMap.size; @@ -105,7 +111,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { // we're out of space, so our call to set will delete this item index = this._cacheMap.peek().index; } - const cacheValue = this._drawToCache(glyph, index); + const cacheValue = this._drawToCache(chars, bg, fg, bold, dim, italic, index); this._cacheMap.set(glyphKey, cacheValue); this._drawFromCache(ctx, cacheValue, x, y); return true; @@ -113,7 +119,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { return false; } - private _canCache(glyph: IGlyphIdentifier): boolean { + private _canCache(code: number): boolean { // Only cache ascii and extended characters for now, to be safe. In the future, we could do // something more complicated to determine the expected width of a character. // @@ -121,7 +127,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { // to draw overlapping glyphs from the atlas: // https://github.com/servo/webrender/issues/464#issuecomment-255632875 // https://webglfundamentals.org/webgl/lessons/webgl-text-texture.html - return glyph.code < 256; + return code < 256; } private _toCoordinates(index: number): [number, number] { @@ -162,39 +168,47 @@ export default class DynamicCharAtlas extends BaseCharAtlas { return DEFAULT_ANSI_COLORS[idx]; } - private _getBackgroundColor(glyph: IGlyphIdentifier): IColor { + private _getBackgroundColor(bg: number): IColor { if (this._config.allowTransparency) { // The background color might have some transparency, so we need to render it as fully // transparent in the atlas. Otherwise we'd end up drawing the transparent background twice // around the anti-aliased edges of the glyph, and it would look too dark. return TRANSPARENT_COLOR; - } else if (glyph.bg === INVERTED_DEFAULT_COLOR) { + } else if (bg === INVERTED_DEFAULT_COLOR) { return this._config.colors.foreground; - } else if (glyph.bg < 256) { - return this._getColorFromAnsiIndex(glyph.bg); + } else if (bg < 256) { + return this._getColorFromAnsiIndex(bg); } return this._config.colors.background; } - private _getForegroundColor(glyph: IGlyphIdentifier): IColor { - if (glyph.fg === INVERTED_DEFAULT_COLOR) { + private _getForegroundColor(fg: number): IColor { + if (fg === INVERTED_DEFAULT_COLOR) { return this._config.colors.background; - } else if (glyph.fg < 256) { + } else if (fg < 256) { // 256 color support - return this._getColorFromAnsiIndex(glyph.fg); + return this._getColorFromAnsiIndex(fg); } return this._config.colors.foreground; } // TODO: We do this (or something similar) in multiple places. We should split this off // into a shared function. - private _drawToCache(glyph: IGlyphIdentifier, index: number): IGlyphCacheValue { + private _drawToCache( + chars: string, + bg: number, + fg: number, + bold: boolean, + dim: boolean, + italic: boolean, + index: number + ): IGlyphCacheValue { this._drawToCacheCount++; this._tmpCtx.save(); // draw the background - const backgroundColor = this._getBackgroundColor(glyph); + const backgroundColor = this._getBackgroundColor(bg); // Use a 'copy' composite operation to clear any existing glyph out of _tmpCtxWithAlpha, regardless of // transparency in backgroundColor this._tmpCtx.globalCompositeOperation = 'copy'; @@ -203,20 +217,20 @@ export default class DynamicCharAtlas extends BaseCharAtlas { this._tmpCtx.globalCompositeOperation = 'source-over'; // draw the foreground/glyph - const fontWeight = glyph.bold ? this._config.fontWeightBold : this._config.fontWeight; - const fontStyle = glyph.italic ? 'italic' : ''; + const fontWeight = bold ? this._config.fontWeightBold : this._config.fontWeight; + const fontStyle = italic ? 'italic' : ''; this._tmpCtx.font = `${fontStyle} ${fontWeight} ${this._config.fontSize * this._config.devicePixelRatio}px ${this._config.fontFamily}`; this._tmpCtx.textBaseline = 'top'; - this._tmpCtx.fillStyle = this._getForegroundColor(glyph).css; + this._tmpCtx.fillStyle = this._getForegroundColor(fg).css; // Apply alpha to dim the character - if (glyph.dim) { + if (dim) { this._tmpCtx.globalAlpha = DIM_OPACITY; } // Draw the character - this._tmpCtx.fillText(glyph.chars, 0, 0); + this._tmpCtx.fillText(chars, 0, 0); this._tmpCtx.restore(); // clear the background from the character to avoid issues with drawing over the previous diff --git a/src/renderer/atlas/NoneCharAtlas.ts b/src/renderer/atlas/NoneCharAtlas.ts index 1cbc9eea..163baf38 100644 --- a/src/renderer/atlas/NoneCharAtlas.ts +++ b/src/renderer/atlas/NoneCharAtlas.ts @@ -5,7 +5,6 @@ * A dummy CharAtlas implementation that always fails to draw characters. */ -import { IGlyphIdentifier } from './Types'; import { ICharAtlasConfig } from '../../shared/atlas/Types'; import BaseCharAtlas from './BaseCharAtlas'; @@ -16,7 +15,13 @@ export default class NoneCharAtlas extends BaseCharAtlas { public draw( ctx: CanvasRenderingContext2D, - glyph: IGlyphIdentifier, + chars: string, + code: number, + bg: number, + fg: number, + bold: boolean, + dim: boolean, + italic: boolean, x: number, y: number ): boolean { diff --git a/src/renderer/atlas/StaticCharAtlas.ts b/src/renderer/atlas/StaticCharAtlas.ts index c0d8a814..0e022fa6 100644 --- a/src/renderer/atlas/StaticCharAtlas.ts +++ b/src/renderer/atlas/StaticCharAtlas.ts @@ -3,7 +3,7 @@ * @license MIT */ -import { DIM_OPACITY, IGlyphIdentifier } from './Types'; +import { DIM_OPACITY } from './Types'; import { CHAR_ATLAS_CELL_SPACING, ICharAtlasConfig } from '../../shared/atlas/Types'; import { generateStaticCharAtlasTexture } from '../../shared/atlas/CharAtlasGenerator'; import BaseCharAtlas from './BaseCharAtlas'; @@ -37,18 +37,24 @@ export default class StaticCharAtlas extends BaseCharAtlas { } } - private _isCached(glyph: IGlyphIdentifier, colorIndex: number): boolean { - const isAscii = glyph.code < 256; + private _isCached(code: number, fg: number, bg: number, italic: boolean): boolean { + const isAscii = code < 256; // A color is basic if it is one of the 4 bit ANSI colors. - const isBasicColor = glyph.fg < 16; - const isDefaultColor = glyph.fg >= 256; - const isDefaultBackground = glyph.bg >= 256; - return isAscii && (isBasicColor || isDefaultColor) && isDefaultBackground && !glyph.italic; + const isBasicColor = fg < 16; + const isDefaultColor = fg >= 256; + const isDefaultBackground = bg >= 256; + return isAscii && (isBasicColor || isDefaultColor) && isDefaultBackground && !italic; } public draw( ctx: CanvasRenderingContext2D, - glyph: IGlyphIdentifier, + chars: string, + code: number, + bg: number, + fg: number, + bold: boolean, + dim: boolean, + italic: boolean, x: number, y: number ): boolean { @@ -58,15 +64,15 @@ export default class StaticCharAtlas extends BaseCharAtlas { } let colorIndex = 0; - if (glyph.fg < 256) { - colorIndex = 2 + glyph.fg + (glyph.bold ? 16 : 0); + if (fg < 256) { + colorIndex = 2 + fg + (bold ? 16 : 0); } else { // If default color and bold - if (glyph.bold) { + if (bold) { colorIndex = 1; } } - if (!this._isCached(glyph, colorIndex)) { + if (!this._isCached(code, fg, bg, italic)) { return false; } @@ -77,13 +83,13 @@ export default class StaticCharAtlas extends BaseCharAtlas { const charAtlasCellHeight = this._config.scaledCharHeight + CHAR_ATLAS_CELL_SPACING; // Apply alpha to dim the character - if (glyph.dim) { + if (dim) { ctx.globalAlpha = DIM_OPACITY; } ctx.drawImage( this._texture, - glyph.code * charAtlasCellWidth, + code * charAtlasCellWidth, colorIndex * charAtlasCellHeight, charAtlasCellWidth, this._config.scaledCharHeight, diff --git a/src/renderer/atlas/Types.ts b/src/renderer/atlas/Types.ts index 6fb3c5d1..34f01d39 100644 --- a/src/renderer/atlas/Types.ts +++ b/src/renderer/atlas/Types.ts @@ -5,13 +5,3 @@ export const INVERTED_DEFAULT_COLOR = -1; export const DIM_OPACITY = 0.5; - -export interface IGlyphIdentifier { - chars: string; - code: number; - bg: number; - fg: number; - bold: boolean; - dim: boolean; - italic: boolean; -} From cf9cb80cb4b82e5dbf11fce59d274175d629b0b8 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 14 Sep 2018 10:08:26 -0700 Subject: [PATCH 02/26] Use a number as the dynamic cache key This prevents string creation on every draw --- src/renderer/atlas/DynamicCharAtlas.ts | 16 ++++++-- src/renderer/atlas/LRUMap.test.ts | 56 +++++++++++++------------- src/renderer/atlas/LRUMap.ts | 8 ++-- 3 files changed, 44 insertions(+), 36 deletions(-) diff --git a/src/renderer/atlas/DynamicCharAtlas.ts b/src/renderer/atlas/DynamicCharAtlas.ts index 66baa822..c3927646 100644 --- a/src/renderer/atlas/DynamicCharAtlas.ts +++ b/src/renderer/atlas/DynamicCharAtlas.ts @@ -34,9 +34,17 @@ interface IGlyphCacheValue { isEmpty: boolean; } -function getGlyphCacheKey(chars: string, fg: number, bg: number, bold: boolean, dim: boolean, italic: boolean): string { - const styleFlags = (bold ? 0 : 4) + (dim ? 0 : 2) + (italic ? 0 : 1); - return `${bg}_${fg}_${styleFlags}${chars}`; +function getGlyphCacheKey(code: number, fg: number, bg: number, bold: boolean, dim: boolean, italic: boolean): number { + // Note that this only returns a valid key when code < 256 + // Layout: + // 0b00000000000000000000000000000001: italic (1) + // 0b00000000000000000000000000000010: dim (1) + // 0b00000000000000000000000000000100: bold (1) + // 0b00000000000000000000111111111000: fg (9) + // 0b00000000000111111111000000000000: bg (9) + // 0b00011111111000000000000000000000: code (8) + // 0b11100000000000000000000000000000: unused (3) + return code << 21 | bg << 12 | fg << 3 | (bold ? 0 : 4) + (dim ? 0 : 2) + (italic ? 0 : 1); } export default class DynamicCharAtlas extends BaseCharAtlas { @@ -98,7 +106,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { x: number, y: number ): boolean { - const glyphKey = getGlyphCacheKey(chars, fg, bg, bold, dim, italic); + const glyphKey = getGlyphCacheKey(code, fg, bg, bold, dim, italic); const cacheValue = this._cacheMap.get(glyphKey); if (cacheValue !== null && cacheValue !== undefined) { this._drawFromCache(ctx, cacheValue, x, y); diff --git a/src/renderer/atlas/LRUMap.test.ts b/src/renderer/atlas/LRUMap.test.ts index ba01e410..197d1159 100644 --- a/src/renderer/atlas/LRUMap.test.ts +++ b/src/renderer/atlas/LRUMap.test.ts @@ -9,57 +9,57 @@ import LRUMap from './LRUMap'; describe('LRUMap', () => { it('can be used to store and retrieve values', () => { const map = new LRUMap(10); - map.set('keya', 'valuea'); - map.set('keyb', 'valueb'); - map.set('keyc', 'valuec'); - assert.strictEqual(map.get('keya'), 'valuea'); - assert.strictEqual(map.get('keyb'), 'valueb'); - assert.strictEqual(map.get('keyc'), 'valuec'); + map.set(1, 'valuea'); + map.set(2, 'valueb'); + map.set(3, 'valuec'); + assert.strictEqual(map.get(1), 'valuea'); + assert.strictEqual(map.get(2), 'valueb'); + assert.strictEqual(map.get(3), 'valuec'); }); it('maintains a size from insertions', () => { const map = new LRUMap(10); assert.strictEqual(map.size, 0); - map.set('a', 'value'); + map.set(1, 'value'); assert.strictEqual(map.size, 1); - map.set('b', 'value'); + map.set(2, 'value'); assert.strictEqual(map.size, 2); }); it('deletes the oldest entry when the capacity is exceeded', () => { const map = new LRUMap(4); - map.set('a', 'value'); - map.set('b', 'value'); - map.set('c', 'value'); - map.set('d', 'value'); - map.set('e', 'value'); - assert.isNull(map.get('a')); - assert.isNotNull(map.get('b')); - assert.isNotNull(map.get('c')); - assert.isNotNull(map.get('d')); - assert.isNotNull(map.get('e')); + map.set(1, 'value'); + map.set(2, 'value'); + map.set(3, 'value'); + map.set(4, 'value'); + map.set(5, 'value'); + assert.isNull(map.get(1)); + assert.isNotNull(map.get(2)); + assert.isNotNull(map.get(3)); + assert.isNotNull(map.get(4)); + assert.isNotNull(map.get(5)); assert.strictEqual(map.size, 4); }); it('prevents a recently accessed entry from getting deleted', () => { const map = new LRUMap(2); - map.set('a', 'value'); - map.set('b', 'value'); - map.get('a'); + map.set(1, 'value'); + map.set(2, 'value'); + map.get(1); // a would normally get deleted here, except that we called get() - map.set('c', 'value'); - assert.isNotNull(map.get('a')); + map.set(3, 'value'); + assert.isNotNull(map.get(1)); // b got deleted instead of a - assert.isNull(map.get('b')); - assert.isNotNull(map.get('c')); + assert.isNull(map.get(2)); + assert.isNotNull(map.get(3)); }); it('supports mutation', () => { const map = new LRUMap(10); - map.set('keya', 'oldvalue'); - map.set('keya', 'newvalue'); + map.set(1, 'oldvalue'); + map.set(1, 'newvalue'); // mutation doesn't change the size assert.strictEqual(map.size, 1); - assert.strictEqual(map.get('keya'), 'newvalue'); + assert.strictEqual(map.get(1), 'newvalue'); }); }); diff --git a/src/renderer/atlas/LRUMap.ts b/src/renderer/atlas/LRUMap.ts index eccfbfea..984dbb72 100644 --- a/src/renderer/atlas/LRUMap.ts +++ b/src/renderer/atlas/LRUMap.ts @@ -6,12 +6,12 @@ interface ILinkedListNode { prev: ILinkedListNode; next: ILinkedListNode; - key: string; + key: number; value: T; } export default class LRUMap { - private _map: { [key: string]: ILinkedListNode } = {}; + private _map: { [key: number]: ILinkedListNode } = {}; private _head: ILinkedListNode = null; private _tail: ILinkedListNode = null; private _nodePool: ILinkedListNode[] = []; @@ -68,7 +68,7 @@ export default class LRUMap { } } - public get(key: string): T | null { + public get(key: number): T | null { // This is unsafe: We're assuming our keyspace doesn't overlap with Object.prototype. However, // it's faster than calling hasOwnProperty, and in our case, it would never overlap. const node = this._map[key]; @@ -85,7 +85,7 @@ export default class LRUMap { return head === null ? null : head.value; } - public set(key: string, value: T): void { + public set(key: number, value: T): void { // This is unsafe: See note above. let node = this._map[key]; if (node !== undefined) { From 6528ba84ffecfe7ca230d40a59c06c97092ac948 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 14 Sep 2018 10:12:43 -0700 Subject: [PATCH 03/26] Avoid creation of array converting index to coordinates --- src/renderer/atlas/DynamicCharAtlas.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/renderer/atlas/DynamicCharAtlas.ts b/src/renderer/atlas/DynamicCharAtlas.ts index c3927646..7476a9f4 100644 --- a/src/renderer/atlas/DynamicCharAtlas.ts +++ b/src/renderer/atlas/DynamicCharAtlas.ts @@ -138,11 +138,12 @@ export default class DynamicCharAtlas extends BaseCharAtlas { return code < 256; } - private _toCoordinates(index: number): [number, number] { - return [ - (index % this._width) * this._config.scaledCharWidth, - Math.floor(index / this._width) * this._config.scaledCharHeight - ]; + private _toCoordinateX(index: number): number { + return (index % this._width) * this._config.scaledCharWidth; + } + + private _toCoordinateY(index: number): number { + return Math.floor(index / this._width) * this._config.scaledCharHeight; } private _drawFromCache( @@ -155,7 +156,8 @@ export default class DynamicCharAtlas extends BaseCharAtlas { if (cacheValue.isEmpty) { return; } - const [cacheX, cacheY] = this._toCoordinates(cacheValue.index); + const cacheX = this._toCoordinateX(cacheValue.index); + const cacheY = this._toCoordinateY(cacheValue.index); ctx.drawImage( this._cacheCanvas, cacheX, @@ -252,7 +254,8 @@ export default class DynamicCharAtlas extends BaseCharAtlas { } // copy the data from imageData to _cacheCanvas - const [x, y] = this._toCoordinates(index); + const x = this._toCoordinateX(index); + const y = this._toCoordinateY(index); // putImageData doesn't do any blending, so it will overwrite any existing cache entry for us this._cacheCtx.putImageData(imageData, x, y); From 7762ab96a5b0d51b8bfcba20b4bf736f35f5a808 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 14 Sep 2018 12:06:14 -0700 Subject: [PATCH 04/26] Exit draw early for the space character --- src/renderer/atlas/DynamicCharAtlas.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/renderer/atlas/DynamicCharAtlas.ts b/src/renderer/atlas/DynamicCharAtlas.ts index 7476a9f4..58aa9a18 100644 --- a/src/renderer/atlas/DynamicCharAtlas.ts +++ b/src/renderer/atlas/DynamicCharAtlas.ts @@ -106,6 +106,11 @@ export default class DynamicCharAtlas extends BaseCharAtlas { x: number, y: number ): boolean { + // Space is always an empty cell, special case this as it's so common + if (code === 32) { + return true; + } + const glyphKey = getGlyphCacheKey(code, fg, bg, bold, dim, italic); const cacheValue = this._cacheMap.get(glyphKey); if (cacheValue !== null && cacheValue !== undefined) { From d0dc387921604b899700478bacec6614ebcdb96b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Fri, 14 Sep 2018 23:16:24 +0200 Subject: [PATCH 05/26] fix overscan issue and enable linkify of partial shown matches --- src/Buffer.ts | 26 +++++++++++++++++++++++--- src/Linkifier.ts | 31 ++++++++++++++++++------------- src/Types.ts | 2 +- 3 files changed, 42 insertions(+), 17 deletions(-) diff --git a/src/Buffer.ts b/src/Buffer.ts index 29b034ac..d7760b11 100644 --- a/src/Buffer.ts +++ b/src/Buffer.ts @@ -371,8 +371,8 @@ export class Buffer implements IBuffer { this.markers.splice(this.markers.indexOf(marker), 1); } - public iterator(trimRight: boolean, startIndex?: number, endIndex?: number): IBufferStringIterator { - return new BufferStringIterator(this, trimRight, startIndex, endIndex); + public iterator(trimRight: boolean, startIndex?: number, endIndex?: number, startOverscan?: number, endOverscan?: number): IBufferStringIterator { + return new BufferStringIterator(this, trimRight, startIndex, endIndex, startOverscan, endOverscan); } } @@ -401,6 +401,18 @@ export class Marker extends EventEmitter implements IMarker { } } +/** + * Iterator to get unwrapped content strings from the buffer. + * The iterator returns at least the string data between the borders + * `startIndex` and `endIndex` (exclusive) and will expand the lines + * by `startOverscan` to the top and by `endOverscan` to the bottom, + * if no new line was found in between. + * It will never read/return string data beyond `startIndex - startOverscan` + * or `endIndex + endOverscan`. Therefore the first and last line might be truncated. + * It is possible to always get the full string for the first and last line as well + * by setting the overscan values to the actual buffer length, but not recommended + * since it might return the whole buffer within a single string in a worst case scenario. + */ export class BufferStringIterator implements IBufferStringIterator { private _current: number; @@ -408,7 +420,9 @@ export class BufferStringIterator implements IBufferStringIterator { private _buffer: IBuffer, private _trimRight: boolean, private _startIndex: number = 0, - private _endIndex: number = _buffer.lines.length + private _endIndex: number = _buffer.lines.length, + private _startOverscan: number = 0, + private _endOverscan: number = 0 ) { this._current = this._startIndex; } @@ -419,6 +433,12 @@ export class BufferStringIterator implements IBufferStringIterator { public next(): IBufferStringIteratorResult { const range = this._buffer.getWrappedRangeForLine(this._current); + if (range.first < this._startIndex - this._startOverscan) { + range.first = this._startIndex - this._startOverscan; + } + if (range.last > this._endIndex + this._endOverscan) { + range.last = this._endIndex + this._endOverscan; + } let result = ''; for (let i = range.first; i <= range.last; ++i) { // TODO: always apply trimRight after fixing #1685 diff --git a/src/Linkifier.ts b/src/Linkifier.ts index 4d84b1a7..b4023bbf 100644 --- a/src/Linkifier.ts +++ b/src/Linkifier.ts @@ -21,6 +21,12 @@ export class Linkifier extends EventEmitter implements ILinkifier { */ protected static readonly TIME_BEFORE_LINKIFY = 200; + /** + * Limit of the unwrapping line expansion (overscan) at the top and bottom + * of the actual viewport. + */ + protected static readonly OVERSCAN_LIMIT = 5; + protected _linkMatchers: ILinkMatcher[] = []; private _mouseZoneManager: IMouseZoneManager; @@ -88,11 +94,17 @@ export class Linkifier extends EventEmitter implements ILinkifier { return; } - // iterate over the range of unwrapped content strings within start..end (excluding) - // _doLinkifyRow gets full unwrapped lines with the start row as buffer offset for every matcher - // for wrapped content over several rows the iterator might return rows outside the viewport - // we skip those later in _doLinkifyRow - const iterator = this._terminal.buffer.iterator(false, absoluteRowIndexStart, this._terminal.buffer.ydisp + this._rowsToLinkify.end + 1); + // Iterate over the range of unwrapped content strings within start..end (excluding). + // _doLinkifyRow gets full unwrapped lines with the start row as buffer offset for every matcher. + // The unwrapping is needed to also match content that got wrapped at the right side. + // To avoid a worst case szenario where the whole buffer contains just a single unwrapped string + // we limit this line expansion beyond the actual viewport to -5 and +5 real buffer lines (overscan). + // This comes with the tradeoff that match longer than 5 buffer lines will not match anymore at the + // viewport borders. + const iterator = this._terminal.buffer.iterator( + false, absoluteRowIndexStart, this._terminal.buffer.ydisp + this._rowsToLinkify.end + 1, + Linkifier.OVERSCAN_LIMIT, Linkifier.OVERSCAN_LIMIT); + console.log('linkify rows', absoluteRowIndexStart, this._terminal.buffer.ydisp + this._rowsToLinkify.end + 1); while (iterator.hasNext()) { const lineData: IBufferStringIteratorResult = iterator.next(); for (let i = 0; i < this._linkMatchers.length; i++) { @@ -204,14 +216,6 @@ export class Linkifier extends EventEmitter implements ILinkifier { // get the buffer index as [absolute row, col] for the match const bufferIndex = this._terminal.buffer.stringIndexToBufferIndex(rowIndex, stringIndex); - // skip rows outside of the viewport - if (bufferIndex[0] - this._terminal.buffer.ydisp < 0) { - continue; - } - if (bufferIndex[0] - this._terminal.buffer.ydisp > this._terminal.rows) { - break; - } - const line = this._terminal.buffer.lines.get(bufferIndex[0]); const char = line.get(bufferIndex[1]); let fg: number | undefined; @@ -254,6 +258,7 @@ export class Linkifier extends EventEmitter implements ILinkifier { x2 = this._terminal.cols; y2--; } + console.log(x1, y1, x2, y2); this._mouseZoneManager.add(new MouseZone( x1 + 1, diff --git a/src/Types.ts b/src/Types.ts index 69c40151..40b3bd02 100644 --- a/src/Types.ts +++ b/src/Types.ts @@ -296,7 +296,7 @@ export interface IBuffer { nextStop(x?: number): number; prevStop(x?: number): number; stringIndexToBufferIndex(lineIndex: number, stringIndex: number): number[]; - iterator(trimRight: boolean, startIndex?: number, endIndex?: number): IBufferStringIterator; + iterator(trimRight: boolean, startIndex?: number, endIndex?: number, startOverscan?: number, endOverscan?: number): IBufferStringIterator; } export interface IBufferSet extends IEventEmitter { From 842ee6c1644d7064bf73c3c2a9cd79b67bbb7753 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Sat, 15 Sep 2018 00:22:00 +0200 Subject: [PATCH 06/26] change overscan limit to 2k chars --- src/Linkifier.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/Linkifier.ts b/src/Linkifier.ts index b4023bbf..fee5401e 100644 --- a/src/Linkifier.ts +++ b/src/Linkifier.ts @@ -23,9 +23,9 @@ export class Linkifier extends EventEmitter implements ILinkifier { /** * Limit of the unwrapping line expansion (overscan) at the top and bottom - * of the actual viewport. + * of the actual viewport in ASCII characters. */ - protected static readonly OVERSCAN_LIMIT = 5; + protected static readonly OVERSCAN_CHAR_LIMIT = 2000; protected _linkMatchers: ILinkMatcher[] = []; @@ -96,15 +96,16 @@ export class Linkifier extends EventEmitter implements ILinkifier { // Iterate over the range of unwrapped content strings within start..end (excluding). // _doLinkifyRow gets full unwrapped lines with the start row as buffer offset for every matcher. - // The unwrapping is needed to also match content that got wrapped at the right side. + // The unwrapping is needed to also match content that got wrapped across several buffer lines. // To avoid a worst case szenario where the whole buffer contains just a single unwrapped string - // we limit this line expansion beyond the actual viewport to -5 and +5 real buffer lines (overscan). - // This comes with the tradeoff that match longer than 5 buffer lines will not match anymore at the - // viewport borders. + // we limit this line expansion beyond the actual viewport to +OVERSCAN_CHAR_LIMIT chars (overscan) + // at the top and the bottom. + // This comes with the tradeoff that matches longer than OVERSCAN_CHAR_LIMIT chars will not match + // anymore at the viewport borders. + const overscanLineLimit = Math.ceil(Linkifier.OVERSCAN_CHAR_LIMIT / this._terminal.cols); const iterator = this._terminal.buffer.iterator( false, absoluteRowIndexStart, this._terminal.buffer.ydisp + this._rowsToLinkify.end + 1, - Linkifier.OVERSCAN_LIMIT, Linkifier.OVERSCAN_LIMIT); - console.log('linkify rows', absoluteRowIndexStart, this._terminal.buffer.ydisp + this._rowsToLinkify.end + 1); + overscanLineLimit, overscanLineLimit); while (iterator.hasNext()) { const lineData: IBufferStringIteratorResult = iterator.next(); for (let i = 0; i < this._linkMatchers.length; i++) { From 5284a5a742b3b6ccc62e89474d51027dbafe81dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Sat, 15 Sep 2018 00:41:55 +0200 Subject: [PATCH 07/26] fix spelling --- src/Buffer.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Buffer.ts b/src/Buffer.ts index d7760b11..815dc84b 100644 --- a/src/Buffer.ts +++ b/src/Buffer.ts @@ -410,7 +410,7 @@ export class Marker extends EventEmitter implements IMarker { * It will never read/return string data beyond `startIndex - startOverscan` * or `endIndex + endOverscan`. Therefore the first and last line might be truncated. * It is possible to always get the full string for the first and last line as well - * by setting the overscan values to the actual buffer length, but not recommended + * by setting the overscan values to the actual buffer length. This not recommended * since it might return the whole buffer within a single string in a worst case scenario. */ export class BufferStringIterator implements IBufferStringIterator { From ac3595e8b41b03181ba49d3da08713e4cc074edd Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 14 Sep 2018 12:05:16 -0700 Subject: [PATCH 08/26] Generate and use a bitmap instead of canvas --- src/renderer/atlas/DynamicCharAtlas.ts | 55 ++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 3 deletions(-) diff --git a/src/renderer/atlas/DynamicCharAtlas.ts b/src/renderer/atlas/DynamicCharAtlas.ts index 58aa9a18..a00bb472 100644 --- a/src/renderer/atlas/DynamicCharAtlas.ts +++ b/src/renderer/atlas/DynamicCharAtlas.ts @@ -29,9 +29,14 @@ const TRANSPARENT_COLOR = { // cache. const FRAME_CACHE_DRAW_LIMIT = 100; +const GLYPH_BITMAP_COMMIT_DELAY = 100; + +const GLYPH_BITMAP_COMMIT_LIMIT = 100; + interface IGlyphCacheValue { index: number; isEmpty: boolean; + inBitmap: boolean; } function getGlyphCacheKey(code: number, fg: number, bg: number, bold: boolean, dim: boolean, italic: boolean): number { @@ -65,6 +70,11 @@ export default class DynamicCharAtlas extends BaseCharAtlas { private _drawToCacheCount: number = 0; + private _glyphsWaitingOnBitmap: Uint32Array = new Uint32Array(GLYPH_BITMAP_COMMIT_LIMIT); + private _glyphsWaitingOnBitmapCount: number = 0; + private _bitmapCommitTimeout: number | null = null; + private _bitmap: ImageBitmap | null = null; + constructor(document: Document, private _config: ICharAtlasConfig) { super(); this._cacheCanvas = document.createElement('canvas'); @@ -124,7 +134,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { // we're out of space, so our call to set will delete this item index = this._cacheMap.peek().index; } - const cacheValue = this._drawToCache(chars, bg, fg, bold, dim, italic, index); + const cacheValue = this._drawToCache(chars, code, bg, fg, bold, dim, italic, index); this._cacheMap.set(glyphKey, cacheValue); this._drawFromCache(ctx, cacheValue, x, y); return true; @@ -164,7 +174,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { const cacheX = this._toCoordinateX(cacheValue.index); const cacheY = this._toCoordinateY(cacheValue.index); ctx.drawImage( - this._cacheCanvas, + cacheValue.inBitmap ? this._bitmap : this._cacheCanvas, cacheX, cacheY, this._config.scaledCharWidth, @@ -211,6 +221,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { // into a shared function. private _drawToCache( chars: string, + code: number, bg: number, fg: number, bold: boolean, @@ -264,9 +275,47 @@ export default class DynamicCharAtlas extends BaseCharAtlas { // putImageData doesn't do any blending, so it will overwrite any existing cache entry for us this._cacheCtx.putImageData(imageData, x, y); + this._glyphsWaitingOnBitmap[this._glyphsWaitingOnBitmapCount++] = getGlyphCacheKey(code, fg, bg, bold, dim, italic); + this._queueGenerateBitmap(); + return { index, - isEmpty + isEmpty, + inBitmap: false }; } + + private _queueGenerateBitmap(): void { + // Check if it's already queued + if (this._bitmapCommitTimeout !== null) { + return; + } + + this._bitmapCommitTimeout = window.setTimeout(() => this._generateBitmap(), GLYPH_BITMAP_COMMIT_DELAY); + } + + private _generateBitmap(): void { + const countAtGeneration = this._glyphsWaitingOnBitmapCount; + // TODO: Fallback when createImageBitmap not supported + window.createImageBitmap(this._cacheCanvas).then(bitmap => { + // Set bitmap + this._bitmap = bitmap; + + // Mark all new glyphs as in bitmap + for (let i = 0; i < countAtGeneration; i++) { + const key = this._glyphsWaitingOnBitmap[i]; + this._cacheMap.get(key).inBitmap = true; + this._glyphsWaitingOnBitmap[i] = 0; + } + + // Fix up any glyphs that were added since image bitmap was created + if (countAtGeneration > this._glyphsWaitingOnBitmapCount) { + // TODO: Verify this + // TODO: Use 2 arrays to speed up set? + this._glyphsWaitingOnBitmap.set(this._glyphsWaitingOnBitmap.subarray(countAtGeneration, this._glyphsWaitingOnBitmapCount - countAtGeneration), 0); + } + this._glyphsWaitingOnBitmapCount -= countAtGeneration; + }); + this._bitmapCommitTimeout = null; + } } From 608d78b6ac2f48fc34269e1a70b82af6cf031ef9 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sat, 15 Sep 2018 09:46:19 -0700 Subject: [PATCH 09/26] Add fallback for window.createImageBitmap --- src/renderer/atlas/DynamicCharAtlas.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/renderer/atlas/DynamicCharAtlas.ts b/src/renderer/atlas/DynamicCharAtlas.ts index a00bb472..4516524f 100644 --- a/src/renderer/atlas/DynamicCharAtlas.ts +++ b/src/renderer/atlas/DynamicCharAtlas.ts @@ -10,6 +10,7 @@ import BaseCharAtlas from './BaseCharAtlas'; import { DEFAULT_ANSI_COLORS } from '../ColorManager'; import { clearColor } from '../../shared/atlas/CharAtlasGenerator'; import LRUMap from './LRUMap'; +import { isFirefox, isSafari } from '../../shared/utils/Browser'; // In practice we're probably never going to exhaust a texture this large. For debugging purposes, // however, it can be useful to set this to a really tiny value, to verify that LRU eviction works. @@ -286,6 +287,14 @@ export default class DynamicCharAtlas extends BaseCharAtlas { } private _queueGenerateBitmap(): void { + // Support is patchy for createImageBitmap at the moment, pass a canvas back + // if support is lacking as drawImage works there too. Firefox is also + // included here as ImageBitmap appears both buggy and has horrible + // performance (tested on v55). + if (!('createImageBitmap' in context) || isFirefox || isSafari) { + return; + } + // Check if it's already queued if (this._bitmapCommitTimeout !== null) { return; @@ -296,7 +305,6 @@ export default class DynamicCharAtlas extends BaseCharAtlas { private _generateBitmap(): void { const countAtGeneration = this._glyphsWaitingOnBitmapCount; - // TODO: Fallback when createImageBitmap not supported window.createImageBitmap(this._cacheCanvas).then(bitmap => { // Set bitmap this._bitmap = bitmap; From 00af63b271f9f29031e0494c0216bc5c6d0c5ee0 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sat, 15 Sep 2018 10:10:50 -0700 Subject: [PATCH 10/26] Expand glyph when needed --- src/renderer/atlas/DynamicCharAtlas.ts | 58 +++++++++++++++++++++----- 1 file changed, 47 insertions(+), 11 deletions(-) diff --git a/src/renderer/atlas/DynamicCharAtlas.ts b/src/renderer/atlas/DynamicCharAtlas.ts index 4516524f..e4f4dfb9 100644 --- a/src/renderer/atlas/DynamicCharAtlas.ts +++ b/src/renderer/atlas/DynamicCharAtlas.ts @@ -30,9 +30,21 @@ const TRANSPARENT_COLOR = { // cache. const FRAME_CACHE_DRAW_LIMIT = 100; +/** + * The number of milliseconds to wait before generating the ImageBitmap, this is to debounce/batch + * the operation as window.createImageBitmap is asynchronous. + */ const GLYPH_BITMAP_COMMIT_DELAY = 100; -const GLYPH_BITMAP_COMMIT_LIMIT = 100; +/** + * The initial size of the queue used to track glyphs waiting on bitmap generation. + */ +const GLYPHS_WAITING_ON_BITMAP_QUEUE_INITIAL_SIZE = 100; + +/** + * When the limit of the bitmap queue is reached, the queue increases by this factor. + */ +const GLYPHS_WAITING_ON_BITMAP_QUEUE_INCREMENT_FACTOR = 2; interface IGlyphCacheValue { index: number; @@ -71,9 +83,16 @@ export default class DynamicCharAtlas extends BaseCharAtlas { private _drawToCacheCount: number = 0; - private _glyphsWaitingOnBitmap: Uint32Array = new Uint32Array(GLYPH_BITMAP_COMMIT_LIMIT); + // An array of glyph keys that are waiting on the bitmap to be generated. + private _glyphsWaitingOnBitmapQueue: Uint32Array = new Uint32Array(GLYPHS_WAITING_ON_BITMAP_QUEUE_INITIAL_SIZE); + + // The number of glyphs keys waiting on the bitmap to be generated. private _glyphsWaitingOnBitmapCount: number = 0; + + // The timeout that is used to batch bitmap generation so it's not requested for every new glyph. private _bitmapCommitTimeout: number | null = null; + + // The bitmap to draw from, this is much faster on other browsers than others. private _bitmap: ImageBitmap | null = null; constructor(document: Document, private _config: ICharAtlasConfig) { @@ -276,8 +295,8 @@ export default class DynamicCharAtlas extends BaseCharAtlas { // putImageData doesn't do any blending, so it will overwrite any existing cache entry for us this._cacheCtx.putImageData(imageData, x, y); - this._glyphsWaitingOnBitmap[this._glyphsWaitingOnBitmapCount++] = getGlyphCacheKey(code, fg, bg, bold, dim, italic); - this._queueGenerateBitmap(); + // Add the glyph and queue it to the bitmap (if the browser supports it) + this._addGlyphToBitmap(code, fg, bg, bold, dim, italic); return { index, @@ -286,7 +305,14 @@ export default class DynamicCharAtlas extends BaseCharAtlas { }; } - private _queueGenerateBitmap(): void { + private _addGlyphToBitmap( + code: number, + bg: number, + fg: number, + bold: boolean, + dim: boolean, + italic: boolean + ): void { // Support is patchy for createImageBitmap at the moment, pass a canvas back // if support is lacking as drawImage works there too. Firefox is also // included here as ImageBitmap appears both buggy and has horrible @@ -295,7 +321,13 @@ export default class DynamicCharAtlas extends BaseCharAtlas { return; } - // Check if it's already queued + // Add the glyph to the queue, increasing the size of it if needed + if (this._glyphsWaitingOnBitmapCount >= this._glyphsWaitingOnBitmapQueue.length) { + this._expandGlyphWaitingOnBitmapQueue(); + } + this._glyphsWaitingOnBitmapQueue[this._glyphsWaitingOnBitmapCount++] = getGlyphCacheKey(code, fg, bg, bold, dim, italic); + + // Check if bitmap generation timeout already exists if (this._bitmapCommitTimeout !== null) { return; } @@ -303,6 +335,12 @@ export default class DynamicCharAtlas extends BaseCharAtlas { this._bitmapCommitTimeout = window.setTimeout(() => this._generateBitmap(), GLYPH_BITMAP_COMMIT_DELAY); } + private _expandGlyphWaitingOnBitmapQueue(): void { + const newQueue = new Uint32Array(this._glyphsWaitingOnBitmapQueue.length * GLYPHS_WAITING_ON_BITMAP_QUEUE_INCREMENT_FACTOR); + newQueue.set(this._glyphsWaitingOnBitmapQueue, 0); + this._glyphsWaitingOnBitmapQueue = newQueue; + } + private _generateBitmap(): void { const countAtGeneration = this._glyphsWaitingOnBitmapCount; window.createImageBitmap(this._cacheCanvas).then(bitmap => { @@ -311,16 +349,14 @@ export default class DynamicCharAtlas extends BaseCharAtlas { // Mark all new glyphs as in bitmap for (let i = 0; i < countAtGeneration; i++) { - const key = this._glyphsWaitingOnBitmap[i]; + const key = this._glyphsWaitingOnBitmapQueue[i]; this._cacheMap.get(key).inBitmap = true; - this._glyphsWaitingOnBitmap[i] = 0; + this._glyphsWaitingOnBitmapQueue[i] = 0; } // Fix up any glyphs that were added since image bitmap was created if (countAtGeneration > this._glyphsWaitingOnBitmapCount) { - // TODO: Verify this - // TODO: Use 2 arrays to speed up set? - this._glyphsWaitingOnBitmap.set(this._glyphsWaitingOnBitmap.subarray(countAtGeneration, this._glyphsWaitingOnBitmapCount - countAtGeneration), 0); + this._glyphsWaitingOnBitmapQueue.set(this._glyphsWaitingOnBitmapQueue.subarray(countAtGeneration, this._glyphsWaitingOnBitmapCount - countAtGeneration), 0); } this._glyphsWaitingOnBitmapCount -= countAtGeneration; }); From 8d4b5cdc55e70e6335b00f7459b03cac8db94fcd Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sat, 15 Sep 2018 10:38:06 -0700 Subject: [PATCH 11/26] Run yarn coveralls after generating report --- azure-pipelines.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/azure-pipelines.yml b/azure-pipelines.yml index 4ab162c8..cba35b39 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -27,6 +27,7 @@ jobs: displayName: 'Lint' - script: | yarn test-coverage + yarn coveralls displayName: 'Generate and publish coverage' - job: macOS From 39047767e4fa5e1453a699900dcf62c9df47cb28 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sat, 15 Sep 2018 10:40:54 -0700 Subject: [PATCH 12/26] Move coverage to mac agent as it's much faster --- azure-pipelines.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/azure-pipelines.yml b/azure-pipelines.yml index cba35b39..342ab832 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -25,10 +25,6 @@ jobs: - script: | yarn lint displayName: 'Lint' - - script: | - yarn test-coverage - yarn coveralls - displayName: 'Generate and publish coverage' - job: macOS pool: @@ -47,6 +43,10 @@ jobs: - script: | yarn lint displayName: 'Lint' + - script: | + yarn test-coverage + yarn coveralls + displayName: 'Generate and publish coverage' - job: Windows pool: From 4d7ce4da460aa3201cbecd57a4c3d1894d397456 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sat, 15 Sep 2018 10:59:41 -0700 Subject: [PATCH 13/26] Set coveralls branch name --- azure-pipelines.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/azure-pipelines.yml b/azure-pipelines.yml index 342ab832..a26bd3d8 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -45,6 +45,7 @@ jobs: displayName: 'Lint' - script: | yarn test-coverage + export COVERALLS_GIT_BRANCH=$BUILD_SOURCEBRANCH yarn coveralls displayName: 'Generate and publish coverage' From 4656bd433cd2ed2c532c1e6183b3c5943bd5e33d Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Mon, 17 Sep 2018 07:37:04 -0700 Subject: [PATCH 14/26] Revert "Remove IGlyphIdentifier in-between object" This reverts commit 04f2d52f26342dc9a88ba65d28e889a43f38ab39. --- src/renderer/BaseRenderLayer.ts | 8 +-- src/renderer/atlas/BaseCharAtlas.ts | 19 ++----- src/renderer/atlas/DynamicCharAtlas.ts | 78 +++++++++----------------- src/renderer/atlas/NoneCharAtlas.ts | 9 +-- src/renderer/atlas/StaticCharAtlas.ts | 34 +++++------ src/renderer/atlas/Types.ts | 10 ++++ 6 files changed, 59 insertions(+), 99 deletions(-) diff --git a/src/renderer/BaseRenderLayer.ts b/src/renderer/BaseRenderLayer.ts index d7755aca..1df9c3ea 100644 --- a/src/renderer/BaseRenderLayer.ts +++ b/src/renderer/BaseRenderLayer.ts @@ -247,13 +247,7 @@ export abstract class BaseRenderLayer implements IRenderLayer { fg += drawInBrightColor ? 8 : 0; const atlasDidDraw = this._charAtlas && this._charAtlas.draw( this._ctx, - chars, - code, - bg, - fg, - bold, - dim, - italic, + {chars, code, bg, fg, bold: bold && terminal.options.enableBold, dim, italic}, x * this._scaledCellWidth + this._scaledCharLeft, y * this._scaledCellHeight + this._scaledCharTop ); diff --git a/src/renderer/atlas/BaseCharAtlas.ts b/src/renderer/atlas/BaseCharAtlas.ts index 325818c6..50d35faa 100644 --- a/src/renderer/atlas/BaseCharAtlas.ts +++ b/src/renderer/atlas/BaseCharAtlas.ts @@ -3,6 +3,8 @@ * @license MIT */ +import { IGlyphIdentifier } from './Types'; + export default abstract class BaseCharAtlas { private _didWarmUp: boolean = false; @@ -37,27 +39,14 @@ export default abstract class BaseCharAtlas { * do nothing and return false in that case. * * @param ctx Where to draw the character onto. - * @param chars The character(s) to draw. This is typically a single character bug can be made up - * of multiple when character joiners are used. - * @param code The character code. - * @param bg The background color. - * @param fg The foreground color. - * @param bold Whether the text is bold. - * @param dim Whether the text is dim. - * @param italic Whether the text is italic. + * @param glyph Information about what to draw * @param x The position on the context to start drawing at * @param y The position on the context to start drawing at * @returns The success state. True if we drew the character. */ public abstract draw( ctx: CanvasRenderingContext2D, - chars: string, - code: number, - bg: number, - fg: number, - bold: boolean, - dim: boolean, - italic: boolean, + glyph: IGlyphIdentifier, x: number, y: number ): boolean; diff --git a/src/renderer/atlas/DynamicCharAtlas.ts b/src/renderer/atlas/DynamicCharAtlas.ts index e4f4dfb9..2e35b8c5 100644 --- a/src/renderer/atlas/DynamicCharAtlas.ts +++ b/src/renderer/atlas/DynamicCharAtlas.ts @@ -3,7 +3,7 @@ * @license MIT */ -import { DIM_OPACITY, INVERTED_DEFAULT_COLOR } from './Types'; +import { DIM_OPACITY, IGlyphIdentifier, INVERTED_DEFAULT_COLOR } from './Types'; import { ICharAtlasConfig } from '../../shared/atlas/Types'; import { IColor } from '../../shared/Types'; import BaseCharAtlas from './BaseCharAtlas'; @@ -52,7 +52,7 @@ interface IGlyphCacheValue { inBitmap: boolean; } -function getGlyphCacheKey(code: number, fg: number, bg: number, bold: boolean, dim: boolean, italic: boolean): number { +function getGlyphCacheKey(glyph: IGlyphIdentifier): number { // Note that this only returns a valid key when code < 256 // Layout: // 0b00000000000000000000000000000001: italic (1) @@ -62,7 +62,7 @@ function getGlyphCacheKey(code: number, fg: number, bg: number, bold: boolean, d // 0b00000000000111111111000000000000: bg (9) // 0b00011111111000000000000000000000: code (8) // 0b11100000000000000000000000000000: unused (3) - return code << 21 | bg << 12 | fg << 3 | (bold ? 0 : 4) + (dim ? 0 : 2) + (italic ? 0 : 1); + return glyph.code << 21 | glyph.bg << 12 | glyph.fg << 3 | (glyph.bold ? 0 : 4) + (glyph.dim ? 0 : 2) + (glyph.italic ? 0 : 1); } export default class DynamicCharAtlas extends BaseCharAtlas { @@ -126,27 +126,21 @@ export default class DynamicCharAtlas extends BaseCharAtlas { public draw( ctx: CanvasRenderingContext2D, - chars: string, - code: number, - bg: number, - fg: number, - bold: boolean, - dim: boolean, - italic: boolean, + glyph: IGlyphIdentifier, x: number, y: number ): boolean { // Space is always an empty cell, special case this as it's so common - if (code === 32) { + if (glyph.code === 32) { return true; } - const glyphKey = getGlyphCacheKey(code, fg, bg, bold, dim, italic); + const glyphKey = getGlyphCacheKey(glyph); const cacheValue = this._cacheMap.get(glyphKey); if (cacheValue !== null && cacheValue !== undefined) { this._drawFromCache(ctx, cacheValue, x, y); return true; - } else if (this._canCache(code) && this._drawToCacheCount < FRAME_CACHE_DRAW_LIMIT) { + } else if (this._canCache(glyph) && this._drawToCacheCount < FRAME_CACHE_DRAW_LIMIT) { let index; if (this._cacheMap.size < this._cacheMap.capacity) { index = this._cacheMap.size; @@ -154,7 +148,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { // we're out of space, so our call to set will delete this item index = this._cacheMap.peek().index; } - const cacheValue = this._drawToCache(chars, code, bg, fg, bold, dim, italic, index); + const cacheValue = this._drawToCache(glyph, index); this._cacheMap.set(glyphKey, cacheValue); this._drawFromCache(ctx, cacheValue, x, y); return true; @@ -162,7 +156,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { return false; } - private _canCache(code: number): boolean { + private _canCache(glyph: IGlyphIdentifier): boolean { // Only cache ascii and extended characters for now, to be safe. In the future, we could do // something more complicated to determine the expected width of a character. // @@ -170,7 +164,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { // to draw overlapping glyphs from the atlas: // https://github.com/servo/webrender/issues/464#issuecomment-255632875 // https://webglfundamentals.org/webgl/lessons/webgl-text-texture.html - return code < 256; + return glyph.code < 256; } private _toCoordinateX(index: number): number { @@ -213,48 +207,39 @@ export default class DynamicCharAtlas extends BaseCharAtlas { return DEFAULT_ANSI_COLORS[idx]; } - private _getBackgroundColor(bg: number): IColor { + private _getBackgroundColor(glyph: IGlyphIdentifier): IColor { if (this._config.allowTransparency) { // The background color might have some transparency, so we need to render it as fully // transparent in the atlas. Otherwise we'd end up drawing the transparent background twice // around the anti-aliased edges of the glyph, and it would look too dark. return TRANSPARENT_COLOR; - } else if (bg === INVERTED_DEFAULT_COLOR) { + } else if (glyph.bg === INVERTED_DEFAULT_COLOR) { return this._config.colors.foreground; - } else if (bg < 256) { - return this._getColorFromAnsiIndex(bg); + } else if (glyph.bg < 256) { + return this._getColorFromAnsiIndex(glyph.bg); } return this._config.colors.background; } - private _getForegroundColor(fg: number): IColor { - if (fg === INVERTED_DEFAULT_COLOR) { + private _getForegroundColor(glyph: IGlyphIdentifier): IColor { + if (glyph.fg === INVERTED_DEFAULT_COLOR) { return this._config.colors.background; - } else if (fg < 256) { + } else if (glyph.fg < 256) { // 256 color support - return this._getColorFromAnsiIndex(fg); + return this._getColorFromAnsiIndex(glyph.fg); } return this._config.colors.foreground; } // TODO: We do this (or something similar) in multiple places. We should split this off // into a shared function. - private _drawToCache( - chars: string, - code: number, - bg: number, - fg: number, - bold: boolean, - dim: boolean, - italic: boolean, - index: number - ): IGlyphCacheValue { + private _drawToCache(glyph: IGlyphIdentifier, index: number): IGlyphCacheValue { this._drawToCacheCount++; this._tmpCtx.save(); // draw the background - const backgroundColor = this._getBackgroundColor(bg); + const backgroundColor = this._getBackgroundColor(glyph); // Use a 'copy' composite operation to clear any existing glyph out of _tmpCtxWithAlpha, regardless of // transparency in backgroundColor this._tmpCtx.globalCompositeOperation = 'copy'; @@ -263,20 +248,20 @@ export default class DynamicCharAtlas extends BaseCharAtlas { this._tmpCtx.globalCompositeOperation = 'source-over'; // draw the foreground/glyph - const fontWeight = bold ? this._config.fontWeightBold : this._config.fontWeight; - const fontStyle = italic ? 'italic' : ''; + const fontWeight = glyph.bold ? this._config.fontWeightBold : this._config.fontWeight; + const fontStyle = glyph.italic ? 'italic' : ''; this._tmpCtx.font = `${fontStyle} ${fontWeight} ${this._config.fontSize * this._config.devicePixelRatio}px ${this._config.fontFamily}`; this._tmpCtx.textBaseline = 'top'; - this._tmpCtx.fillStyle = this._getForegroundColor(fg).css; + this._tmpCtx.fillStyle = this._getForegroundColor(glyph).css; // Apply alpha to dim the character - if (dim) { + if (glyph.dim) { this._tmpCtx.globalAlpha = DIM_OPACITY; } // Draw the character - this._tmpCtx.fillText(chars, 0, 0); + this._tmpCtx.fillText(glyph.chars, 0, 0); this._tmpCtx.restore(); // clear the background from the character to avoid issues with drawing over the previous @@ -296,7 +281,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { this._cacheCtx.putImageData(imageData, x, y); // Add the glyph and queue it to the bitmap (if the browser supports it) - this._addGlyphToBitmap(code, fg, bg, bold, dim, italic); + this._addGlyphToBitmap(glyph); return { index, @@ -305,14 +290,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { }; } - private _addGlyphToBitmap( - code: number, - bg: number, - fg: number, - bold: boolean, - dim: boolean, - italic: boolean - ): void { + private _addGlyphToBitmap(glyph: IGlyphIdentifier): void { // Support is patchy for createImageBitmap at the moment, pass a canvas back // if support is lacking as drawImage works there too. Firefox is also // included here as ImageBitmap appears both buggy and has horrible @@ -325,7 +303,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { if (this._glyphsWaitingOnBitmapCount >= this._glyphsWaitingOnBitmapQueue.length) { this._expandGlyphWaitingOnBitmapQueue(); } - this._glyphsWaitingOnBitmapQueue[this._glyphsWaitingOnBitmapCount++] = getGlyphCacheKey(code, fg, bg, bold, dim, italic); + this._glyphsWaitingOnBitmapQueue[this._glyphsWaitingOnBitmapCount++] = getGlyphCacheKey(glyph); // Check if bitmap generation timeout already exists if (this._bitmapCommitTimeout !== null) { diff --git a/src/renderer/atlas/NoneCharAtlas.ts b/src/renderer/atlas/NoneCharAtlas.ts index 163baf38..1cbc9eea 100644 --- a/src/renderer/atlas/NoneCharAtlas.ts +++ b/src/renderer/atlas/NoneCharAtlas.ts @@ -5,6 +5,7 @@ * A dummy CharAtlas implementation that always fails to draw characters. */ +import { IGlyphIdentifier } from './Types'; import { ICharAtlasConfig } from '../../shared/atlas/Types'; import BaseCharAtlas from './BaseCharAtlas'; @@ -15,13 +16,7 @@ export default class NoneCharAtlas extends BaseCharAtlas { public draw( ctx: CanvasRenderingContext2D, - chars: string, - code: number, - bg: number, - fg: number, - bold: boolean, - dim: boolean, - italic: boolean, + glyph: IGlyphIdentifier, x: number, y: number ): boolean { diff --git a/src/renderer/atlas/StaticCharAtlas.ts b/src/renderer/atlas/StaticCharAtlas.ts index 0e022fa6..c0d8a814 100644 --- a/src/renderer/atlas/StaticCharAtlas.ts +++ b/src/renderer/atlas/StaticCharAtlas.ts @@ -3,7 +3,7 @@ * @license MIT */ -import { DIM_OPACITY } from './Types'; +import { DIM_OPACITY, IGlyphIdentifier } from './Types'; import { CHAR_ATLAS_CELL_SPACING, ICharAtlasConfig } from '../../shared/atlas/Types'; import { generateStaticCharAtlasTexture } from '../../shared/atlas/CharAtlasGenerator'; import BaseCharAtlas from './BaseCharAtlas'; @@ -37,24 +37,18 @@ export default class StaticCharAtlas extends BaseCharAtlas { } } - private _isCached(code: number, fg: number, bg: number, italic: boolean): boolean { - const isAscii = code < 256; + private _isCached(glyph: IGlyphIdentifier, colorIndex: number): boolean { + const isAscii = glyph.code < 256; // A color is basic if it is one of the 4 bit ANSI colors. - const isBasicColor = fg < 16; - const isDefaultColor = fg >= 256; - const isDefaultBackground = bg >= 256; - return isAscii && (isBasicColor || isDefaultColor) && isDefaultBackground && !italic; + const isBasicColor = glyph.fg < 16; + const isDefaultColor = glyph.fg >= 256; + const isDefaultBackground = glyph.bg >= 256; + return isAscii && (isBasicColor || isDefaultColor) && isDefaultBackground && !glyph.italic; } public draw( ctx: CanvasRenderingContext2D, - chars: string, - code: number, - bg: number, - fg: number, - bold: boolean, - dim: boolean, - italic: boolean, + glyph: IGlyphIdentifier, x: number, y: number ): boolean { @@ -64,15 +58,15 @@ export default class StaticCharAtlas extends BaseCharAtlas { } let colorIndex = 0; - if (fg < 256) { - colorIndex = 2 + fg + (bold ? 16 : 0); + if (glyph.fg < 256) { + colorIndex = 2 + glyph.fg + (glyph.bold ? 16 : 0); } else { // If default color and bold - if (bold) { + if (glyph.bold) { colorIndex = 1; } } - if (!this._isCached(code, fg, bg, italic)) { + if (!this._isCached(glyph, colorIndex)) { return false; } @@ -83,13 +77,13 @@ export default class StaticCharAtlas extends BaseCharAtlas { const charAtlasCellHeight = this._config.scaledCharHeight + CHAR_ATLAS_CELL_SPACING; // Apply alpha to dim the character - if (dim) { + if (glyph.dim) { ctx.globalAlpha = DIM_OPACITY; } ctx.drawImage( this._texture, - code * charAtlasCellWidth, + glyph.code * charAtlasCellWidth, colorIndex * charAtlasCellHeight, charAtlasCellWidth, this._config.scaledCharHeight, diff --git a/src/renderer/atlas/Types.ts b/src/renderer/atlas/Types.ts index 34f01d39..6fb3c5d1 100644 --- a/src/renderer/atlas/Types.ts +++ b/src/renderer/atlas/Types.ts @@ -5,3 +5,13 @@ export const INVERTED_DEFAULT_COLOR = -1; export const DIM_OPACITY = 0.5; + +export interface IGlyphIdentifier { + chars: string; + code: number; + bg: number; + fg: number; + bold: boolean; + dim: boolean; + italic: boolean; +} From 202734f10c2fea3b75dba03dd804f00fa9798767 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Mon, 17 Sep 2018 07:43:10 -0700 Subject: [PATCH 15/26] Fix typo which broke dynamic atlas --- src/renderer/atlas/DynamicCharAtlas.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderer/atlas/DynamicCharAtlas.ts b/src/renderer/atlas/DynamicCharAtlas.ts index 2e35b8c5..e025ef34 100644 --- a/src/renderer/atlas/DynamicCharAtlas.ts +++ b/src/renderer/atlas/DynamicCharAtlas.ts @@ -295,7 +295,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { // if support is lacking as drawImage works there too. Firefox is also // included here as ImageBitmap appears both buggy and has horrible // performance (tested on v55). - if (!('createImageBitmap' in context) || isFirefox || isSafari) { + if (!('createImageBitmap' in window) || isFirefox || isSafari) { return; } From 94b20d40bf298576fc207ea583a01fe21ddfd7a2 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Mon, 17 Sep 2018 07:43:31 -0700 Subject: [PATCH 16/26] Reuse object for glyph identifier --- src/renderer/BaseRenderLayer.ts | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/renderer/BaseRenderLayer.ts b/src/renderer/BaseRenderLayer.ts index 1df9c3ea..55cc747e 100644 --- a/src/renderer/BaseRenderLayer.ts +++ b/src/renderer/BaseRenderLayer.ts @@ -5,7 +5,7 @@ import { IRenderLayer, IColorSet, IRenderDimensions } from './Types'; import { CharData, ITerminal } from '../Types'; -import { DIM_OPACITY, INVERTED_DEFAULT_COLOR } from './atlas/Types'; +import { DIM_OPACITY, INVERTED_DEFAULT_COLOR, IGlyphIdentifier } from './atlas/Types'; import BaseCharAtlas from './atlas/BaseCharAtlas'; import { acquireCharAtlas } from './atlas/CharAtlasCache'; import { CHAR_DATA_CHAR_INDEX } from '../Buffer'; @@ -22,6 +22,19 @@ export abstract class BaseRenderLayer implements IRenderLayer { protected _charAtlas: BaseCharAtlas; + /** + * An object that's reused when drawing glyphs in order to reduce GC. + */ + private _currentGlyphIdentifier: IGlyphIdentifier = { + chars: '', + code: 0, + bg: 0, + fg: 0, + bold: false, + dim: false, + italic: false + }; + constructor( private _container: HTMLElement, id: string, @@ -245,9 +258,16 @@ export abstract class BaseRenderLayer implements IRenderLayer { const drawInBrightColor = terminal.options.drawBoldTextInBrightColors && bold && fg < 8 && fg !== INVERTED_DEFAULT_COLOR; fg += drawInBrightColor ? 8 : 0; + this._currentGlyphIdentifier.chars = chars; + this._currentGlyphIdentifier.code = code; + this._currentGlyphIdentifier.bg = bg; + this._currentGlyphIdentifier.fg = fg; + this._currentGlyphIdentifier.bold = bold && terminal.options.enableBold; + this._currentGlyphIdentifier.dim = dim; + this._currentGlyphIdentifier.italic = italic; const atlasDidDraw = this._charAtlas && this._charAtlas.draw( this._ctx, - {chars, code, bg, fg, bold: bold && terminal.options.enableBold, dim, italic}, + this._currentGlyphIdentifier, x * this._scaledCellWidth + this._scaledCharLeft, y * this._scaledCellHeight + this._scaledCharTop ); From d9ec47ab7aaa37bdc773edab29f0226a14f57fa2 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Mon, 17 Sep 2018 07:48:05 -0700 Subject: [PATCH 17/26] Clear bitmap timeout on dispose --- src/renderer/BaseRenderLayer.ts | 1 + src/renderer/atlas/BaseCharAtlas.ts | 5 ++++- src/renderer/atlas/DynamicCharAtlas.ts | 7 +++++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/renderer/BaseRenderLayer.ts b/src/renderer/BaseRenderLayer.ts index 55cc747e..1590d6e2 100644 --- a/src/renderer/BaseRenderLayer.ts +++ b/src/renderer/BaseRenderLayer.ts @@ -51,6 +51,7 @@ export abstract class BaseRenderLayer implements IRenderLayer { public dispose(): void { this._container.removeChild(this._canvas); + this._charAtlas.dispose(); } private _initCanvas(): void { diff --git a/src/renderer/atlas/BaseCharAtlas.ts b/src/renderer/atlas/BaseCharAtlas.ts index 50d35faa..ee69b381 100644 --- a/src/renderer/atlas/BaseCharAtlas.ts +++ b/src/renderer/atlas/BaseCharAtlas.ts @@ -4,10 +4,13 @@ */ import { IGlyphIdentifier } from './Types'; +import { IDisposable } from 'xterm'; -export default abstract class BaseCharAtlas { +export default abstract class BaseCharAtlas implements IDisposable { private _didWarmUp: boolean = false; + public dispose(): void { } + /** * Perform any work needed to warm the cache before it can be used. May be called multiple times. * Implement _doWarmUp instead if you only want to get called once. diff --git a/src/renderer/atlas/DynamicCharAtlas.ts b/src/renderer/atlas/DynamicCharAtlas.ts index e025ef34..31deff55 100644 --- a/src/renderer/atlas/DynamicCharAtlas.ts +++ b/src/renderer/atlas/DynamicCharAtlas.ts @@ -120,6 +120,13 @@ export default class DynamicCharAtlas extends BaseCharAtlas { // document.body.appendChild(this._cacheCanvas); } + public dispose(): void { + if (this._bitmapCommitTimeout !== null) { + window.clearTimeout(this._bitmapCommitTimeout); + this._bitmapCommitTimeout = null; + } + } + public beginFrame(): void { this._drawToCacheCount = 0; } From cbe7b21090b7744b5ab3116b20db0bd12215c8d3 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Mon, 17 Sep 2018 07:52:37 -0700 Subject: [PATCH 18/26] Don't unlink node when updating inBitmap and verify undefined --- src/renderer/atlas/DynamicCharAtlas.ts | 6 +++++- src/renderer/atlas/LRUMap.ts | 11 +++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/renderer/atlas/DynamicCharAtlas.ts b/src/renderer/atlas/DynamicCharAtlas.ts index 31deff55..04d28c87 100644 --- a/src/renderer/atlas/DynamicCharAtlas.ts +++ b/src/renderer/atlas/DynamicCharAtlas.ts @@ -335,7 +335,11 @@ export default class DynamicCharAtlas extends BaseCharAtlas { // Mark all new glyphs as in bitmap for (let i = 0; i < countAtGeneration; i++) { const key = this._glyphsWaitingOnBitmapQueue[i]; - this._cacheMap.get(key).inBitmap = true; + const value = this._cacheMap.peekValue(key); + // If the value has already been evicted, do nothing + if (value) { + value.inBitmap = true; + } this._glyphsWaitingOnBitmapQueue[i] = 0; } diff --git a/src/renderer/atlas/LRUMap.ts b/src/renderer/atlas/LRUMap.ts index 984dbb72..d7e01ec6 100644 --- a/src/renderer/atlas/LRUMap.ts +++ b/src/renderer/atlas/LRUMap.ts @@ -80,6 +80,17 @@ export default class LRUMap { return null; } + /** + * Gets a value from a key without marking it as the most recently used item. + */ + public peekValue(key: number): T | null { + const node = this._map[key]; + if (node !== undefined) { + return node.value; + } + return null; + } + public peek(): T | null { const head = this._head; return head === null ? null : head.value; From 39a7b18065647e289419a0824147f6af1be85900 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Thu, 20 Sep 2018 10:09:39 -0700 Subject: [PATCH 19/26] Keep track of glyphs using cache values, not keys --- src/renderer/atlas/DynamicCharAtlas.ts | 52 +++++++------------------- 1 file changed, 14 insertions(+), 38 deletions(-) diff --git a/src/renderer/atlas/DynamicCharAtlas.ts b/src/renderer/atlas/DynamicCharAtlas.ts index 04d28c87..d8654784 100644 --- a/src/renderer/atlas/DynamicCharAtlas.ts +++ b/src/renderer/atlas/DynamicCharAtlas.ts @@ -36,16 +36,6 @@ const FRAME_CACHE_DRAW_LIMIT = 100; */ const GLYPH_BITMAP_COMMIT_DELAY = 100; -/** - * The initial size of the queue used to track glyphs waiting on bitmap generation. - */ -const GLYPHS_WAITING_ON_BITMAP_QUEUE_INITIAL_SIZE = 100; - -/** - * When the limit of the bitmap queue is reached, the queue increases by this factor. - */ -const GLYPHS_WAITING_ON_BITMAP_QUEUE_INCREMENT_FACTOR = 2; - interface IGlyphCacheValue { index: number; isEmpty: boolean; @@ -84,7 +74,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { private _drawToCacheCount: number = 0; // An array of glyph keys that are waiting on the bitmap to be generated. - private _glyphsWaitingOnBitmapQueue: Uint32Array = new Uint32Array(GLYPHS_WAITING_ON_BITMAP_QUEUE_INITIAL_SIZE); + private _glyphsWaitingOnBitmapQueue: IGlyphCacheValue[] = []; // The number of glyphs keys waiting on the bitmap to be generated. private _glyphsWaitingOnBitmapCount: number = 0; @@ -288,16 +278,17 @@ export default class DynamicCharAtlas extends BaseCharAtlas { this._cacheCtx.putImageData(imageData, x, y); // Add the glyph and queue it to the bitmap (if the browser supports it) - this._addGlyphToBitmap(glyph); - - return { + const cacheValue = { index, isEmpty, inBitmap: false }; + this._addGlyphToBitmap(cacheValue); + + return cacheValue; } - private _addGlyphToBitmap(glyph: IGlyphIdentifier): void { + private _addGlyphToBitmap(cacheValue: IGlyphCacheValue): void { // Support is patchy for createImageBitmap at the moment, pass a canvas back // if support is lacking as drawImage works there too. Firefox is also // included here as ImageBitmap appears both buggy and has horrible @@ -306,11 +297,9 @@ export default class DynamicCharAtlas extends BaseCharAtlas { return; } - // Add the glyph to the queue, increasing the size of it if needed - if (this._glyphsWaitingOnBitmapCount >= this._glyphsWaitingOnBitmapQueue.length) { - this._expandGlyphWaitingOnBitmapQueue(); - } - this._glyphsWaitingOnBitmapQueue[this._glyphsWaitingOnBitmapCount++] = getGlyphCacheKey(glyph); + // Add the glyph to the queue + this._glyphsWaitingOnBitmapQueue.push(cacheValue); + this._glyphsWaitingOnBitmapCount++; // Check if bitmap generation timeout already exists if (this._bitmapCommitTimeout !== null) { @@ -320,34 +309,21 @@ export default class DynamicCharAtlas extends BaseCharAtlas { this._bitmapCommitTimeout = window.setTimeout(() => this._generateBitmap(), GLYPH_BITMAP_COMMIT_DELAY); } - private _expandGlyphWaitingOnBitmapQueue(): void { - const newQueue = new Uint32Array(this._glyphsWaitingOnBitmapQueue.length * GLYPHS_WAITING_ON_BITMAP_QUEUE_INCREMENT_FACTOR); - newQueue.set(this._glyphsWaitingOnBitmapQueue, 0); - this._glyphsWaitingOnBitmapQueue = newQueue; - } - private _generateBitmap(): void { - const countAtGeneration = this._glyphsWaitingOnBitmapCount; + let countAtGeneration = this._glyphsWaitingOnBitmapCount; window.createImageBitmap(this._cacheCanvas).then(bitmap => { // Set bitmap this._bitmap = bitmap; - // Mark all new glyphs as in bitmap - for (let i = 0; i < countAtGeneration; i++) { - const key = this._glyphsWaitingOnBitmapQueue[i]; - const value = this._cacheMap.peekValue(key); + // Mark all new glyphs as in bitmap, excluding glyphs that came in after + // the bitmap was requested + while (countAtGeneration-- > 0) { + const value = this._glyphsWaitingOnBitmapQueue[0]; // If the value has already been evicted, do nothing if (value) { value.inBitmap = true; } - this._glyphsWaitingOnBitmapQueue[i] = 0; } - - // Fix up any glyphs that were added since image bitmap was created - if (countAtGeneration > this._glyphsWaitingOnBitmapCount) { - this._glyphsWaitingOnBitmapQueue.set(this._glyphsWaitingOnBitmapQueue.subarray(countAtGeneration, this._glyphsWaitingOnBitmapCount - countAtGeneration), 0); - } - this._glyphsWaitingOnBitmapCount -= countAtGeneration; }); this._bitmapCommitTimeout = null; } From 10687b138d04c77013ec24cd71b5b885a0e2f9d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Fri, 21 Sep 2018 20:23:41 +0200 Subject: [PATCH 20/26] range checks for buffer iterator --- src/Buffer.test.ts | 33 ++++++++++++++++++++++++++++++++- src/Buffer.ts | 10 ++++++++++ src/Linkifier.ts | 1 - 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/src/Buffer.test.ts b/src/Buffer.test.ts index 32492ee0..0dcdd02d 100644 --- a/src/Buffer.test.ts +++ b/src/Buffer.test.ts @@ -3,7 +3,7 @@ * @license MIT */ -import { assert } from 'chai'; +import { assert, expect } from 'chai'; import { ITerminal } from './Types'; import { Buffer, DEFAULT_ATTR, CHAR_DATA_CHAR_INDEX } from './Buffer'; import { CircularList } from './common/CircularList'; @@ -514,4 +514,35 @@ describe('Buffer', () => { } }); }); + describe('BufferStringIterator', function(): void { + it('iterator does not ovrflow buffer limits', function(): void { + const terminal = new TestTerminal({rows: 5, cols: 10, scrollback: 5}); + const data = [ + 'aaaaaaaaaa', + 'aaaaaaaaa\n', + 'aaaaaaaaaa', + 'aaaaaaaaa\n', + 'aaaaaaaaaa', + 'aaaaaaaaaa', + 'aaaaaaaaaa', + 'aaaaaaaaa\n', + 'aaaaaaaaaa', + 'aaaaaaaaaa' + ]; + terminal.writeSync(data.join('')); + // brute force test with insane values + expect(() => { + for (let overscan = 0; overscan < 20; ++overscan) { + for (let start = -10; start < 20; ++start) { + for (let end = -10; end < 20; ++end) { + const it = terminal.buffer.iterator(false, start, end, overscan, overscan); + while (it.hasNext()) { + it.next(); + } + } + } + } + }).to.not.throw(); + }); + }); }); diff --git a/src/Buffer.ts b/src/Buffer.ts index 815dc84b..be5a47f1 100644 --- a/src/Buffer.ts +++ b/src/Buffer.ts @@ -424,6 +424,12 @@ export class BufferStringIterator implements IBufferStringIterator { private _startOverscan: number = 0, private _endOverscan: number = 0 ) { + if (this._startIndex < 0) { + this._startIndex = 0; + } + if (this._endIndex > this._buffer.lines.length) { + this._endIndex = this._buffer.lines.length; + } this._current = this._startIndex; } @@ -433,12 +439,16 @@ export class BufferStringIterator implements IBufferStringIterator { public next(): IBufferStringIteratorResult { const range = this._buffer.getWrappedRangeForLine(this._current); + // limit search window to overscan value at both borders if (range.first < this._startIndex - this._startOverscan) { range.first = this._startIndex - this._startOverscan; } if (range.last > this._endIndex + this._endOverscan) { range.last = this._endIndex + this._endOverscan; } + // limit to current buffer length + range.first = Math.max(range.first, 0); + range.last = Math.min(range.last, this._buffer.lines.length); let result = ''; for (let i = range.first; i <= range.last; ++i) { // TODO: always apply trimRight after fixing #1685 diff --git a/src/Linkifier.ts b/src/Linkifier.ts index 3be89f80..7d39d146 100644 --- a/src/Linkifier.ts +++ b/src/Linkifier.ts @@ -262,7 +262,6 @@ export class Linkifier extends EventEmitter implements ILinkifier { x2 = this._terminal.cols; y2--; } - console.log(x1, y1, x2, y2); this._mouseZoneManager.add(new MouseZone( x1 + 1, From a30bb22c7a222656df37c98e17068ba110c4276f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Sat, 22 Sep 2018 15:20:31 +0200 Subject: [PATCH 21/26] fix comments to <80 --- src/Linkifier.ts | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/Linkifier.ts b/src/Linkifier.ts index 7d39d146..e5158e73 100644 --- a/src/Linkifier.ts +++ b/src/Linkifier.ts @@ -24,6 +24,7 @@ export class Linkifier extends EventEmitter implements ILinkifier { /** * Limit of the unwrapping line expansion (overscan) at the top and bottom * of the actual viewport in ASCII characters. + * A limit of 2000 should match most sane urls. */ protected static readonly OVERSCAN_CHAR_LIMIT = 2000; @@ -98,14 +99,16 @@ export class Linkifier extends EventEmitter implements ILinkifier { // Invalidate bad end row values (if a resize happened) const absoluteRowIndexEnd = buffer.ydisp + Math.min(this._rowsToLinkify.end, this._terminal.rows) + 1; - // Iterate over the range of unwrapped content strings within start..end (excluding). - // _doLinkifyRow gets full unwrapped lines with the start row as buffer offset for every matcher. - // The unwrapping is needed to also match content that got wrapped across several buffer lines. - // To avoid a worst case szenario where the whole buffer contains just a single unwrapped string - // we limit this line expansion beyond the actual viewport to +OVERSCAN_CHAR_LIMIT chars (overscan) - // at the top and the bottom. - // This comes with the tradeoff that matches longer than OVERSCAN_CHAR_LIMIT chars will not match - // anymore at the viewport borders. + // Iterate over the range of unwrapped content strings within start..end + // (excluding). + // _doLinkifyRow gets full unwrapped lines with the start row as buffer offset + // for every matcher. + // The unwrapping is needed to also match content that got wrapped across + // several buffer lines. To avoid a worst case szenario where the whole buffer + // contains just a single unwrapped string we limit this line expansion beyond + // the viewport to +OVERSCAN_CHAR_LIMIT chars (overscan) at top and bottom. + // This comes with the tradeoff that matches longer than OVERSCAN_CHAR_LIMIT + // chars will not match anymore at the viewport borders. const overscanLineLimit = Math.ceil(Linkifier.OVERSCAN_CHAR_LIMIT / this._terminal.cols); const iterator = this._terminal.buffer.iterator( false, absoluteRowIndexStart, absoluteRowIndexEnd, overscanLineLimit, overscanLineLimit); From 750ec3870cf441cfe3e518f20e85983939d2882a Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sat, 22 Sep 2018 13:15:39 -0700 Subject: [PATCH 22/26] Resolve feedback --- src/renderer/atlas/DynamicCharAtlas.ts | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/renderer/atlas/DynamicCharAtlas.ts b/src/renderer/atlas/DynamicCharAtlas.ts index d8654784..f8539bef 100644 --- a/src/renderer/atlas/DynamicCharAtlas.ts +++ b/src/renderer/atlas/DynamicCharAtlas.ts @@ -76,9 +76,6 @@ export default class DynamicCharAtlas extends BaseCharAtlas { // An array of glyph keys that are waiting on the bitmap to be generated. private _glyphsWaitingOnBitmapQueue: IGlyphCacheValue[] = []; - // The number of glyphs keys waiting on the bitmap to be generated. - private _glyphsWaitingOnBitmapCount: number = 0; - // The timeout that is used to batch bitmap generation so it's not requested for every new glyph. private _bitmapCommitTimeout: number | null = null; @@ -299,7 +296,6 @@ export default class DynamicCharAtlas extends BaseCharAtlas { // Add the glyph to the queue this._glyphsWaitingOnBitmapQueue.push(cacheValue); - this._glyphsWaitingOnBitmapCount++; // Check if bitmap generation timeout already exists if (this._bitmapCommitTimeout !== null) { @@ -310,7 +306,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { } private _generateBitmap(): void { - let countAtGeneration = this._glyphsWaitingOnBitmapCount; + let countAtGeneration = this._glyphsWaitingOnBitmapQueue.length; window.createImageBitmap(this._cacheCanvas).then(bitmap => { // Set bitmap this._bitmap = bitmap; @@ -318,11 +314,10 @@ export default class DynamicCharAtlas extends BaseCharAtlas { // Mark all new glyphs as in bitmap, excluding glyphs that came in after // the bitmap was requested while (countAtGeneration-- > 0) { - const value = this._glyphsWaitingOnBitmapQueue[0]; - // If the value has already been evicted, do nothing - if (value) { - value.inBitmap = true; - } + const value = this._glyphsWaitingOnBitmapQueue.shift(); + // It doesn't matter if the value was already evicted, it will be + // released from memory after this block if so. + value.inBitmap = true; } }); this._bitmapCommitTimeout = null; From 16c0ad858ae7dc21b76f62d6e845272b10b3aeae Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sat, 22 Sep 2018 13:23:22 -0700 Subject: [PATCH 23/26] Make a new empty array instead of shift --- src/renderer/atlas/DynamicCharAtlas.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/renderer/atlas/DynamicCharAtlas.ts b/src/renderer/atlas/DynamicCharAtlas.ts index f8539bef..e67fe4f0 100644 --- a/src/renderer/atlas/DynamicCharAtlas.ts +++ b/src/renderer/atlas/DynamicCharAtlas.ts @@ -74,7 +74,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { private _drawToCacheCount: number = 0; // An array of glyph keys that are waiting on the bitmap to be generated. - private _glyphsWaitingOnBitmapQueue: IGlyphCacheValue[] = []; + private _glyphsWaitingOnBitmap: IGlyphCacheValue[] = []; // The timeout that is used to batch bitmap generation so it's not requested for every new glyph. private _bitmapCommitTimeout: number | null = null; @@ -295,7 +295,7 @@ export default class DynamicCharAtlas extends BaseCharAtlas { } // Add the glyph to the queue - this._glyphsWaitingOnBitmapQueue.push(cacheValue); + this._glyphsWaitingOnBitmap.push(cacheValue); // Check if bitmap generation timeout already exists if (this._bitmapCommitTimeout !== null) { @@ -306,15 +306,16 @@ export default class DynamicCharAtlas extends BaseCharAtlas { } private _generateBitmap(): void { - let countAtGeneration = this._glyphsWaitingOnBitmapQueue.length; + const glyphsMovingToBitmap = this._glyphsWaitingOnBitmap; + this._glyphsWaitingOnBitmap = []; window.createImageBitmap(this._cacheCanvas).then(bitmap => { // Set bitmap this._bitmap = bitmap; // Mark all new glyphs as in bitmap, excluding glyphs that came in after // the bitmap was requested - while (countAtGeneration-- > 0) { - const value = this._glyphsWaitingOnBitmapQueue.shift(); + for (let i = 0; i < glyphsMovingToBitmap.length; i++) { + const value = glyphsMovingToBitmap[i]; // It doesn't matter if the value was already evicted, it will be // released from memory after this block if so. value.inBitmap = true; From fefc6c6a344ccc647109ea5dfe544b571ff131de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Wed, 26 Sep 2018 22:07:02 +0200 Subject: [PATCH 24/26] fix typo --- src/Linkifier.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Linkifier.ts b/src/Linkifier.ts index e5158e73..2ec3ed40 100644 --- a/src/Linkifier.ts +++ b/src/Linkifier.ts @@ -104,7 +104,7 @@ export class Linkifier extends EventEmitter implements ILinkifier { // _doLinkifyRow gets full unwrapped lines with the start row as buffer offset // for every matcher. // The unwrapping is needed to also match content that got wrapped across - // several buffer lines. To avoid a worst case szenario where the whole buffer + // several buffer lines. To avoid a worst case scenario where the whole buffer // contains just a single unwrapped string we limit this line expansion beyond // the viewport to +OVERSCAN_CHAR_LIMIT chars (overscan) at top and bottom. // This comes with the tradeoff that matches longer than OVERSCAN_CHAR_LIMIT From b28bba56df88691d7228108a056e7e7a5e4a64b7 Mon Sep 17 00:00:00 2001 From: Sebastian Pfitzner Date: Thu, 27 Sep 2018 15:46:33 +0200 Subject: [PATCH 25/26] Add new use case: Juno --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index f2f16c98..6b9b877c 100644 --- a/README.md +++ b/README.md @@ -170,6 +170,7 @@ computational environment for Jupyter, supporting interactive data science and s - [**Nutanix**](https://github.com/nutanix): Nutanix Enterprise Cloud uses xterm in the webssh functionality within Nutanix Calm, and is also looking to move our old noserial (termjs) functionality to xterm.js - [**SSH Web Client**](https://github.com/roke22/PHP-SSH2-Web-Client): SSH Web Client with PHP. - [**Shellvault**](https://www.shellvault.io): The cloud-based SSH terminal you can access from anywhere. +- [**Juno**](http://junolab.org/): A flexible Julia IDE, based on Atom. [And much more...](https://github.com/xtermjs/xterm.js/network/dependents) From 5a1c71c1d5637d87d8babb45e66c1c3d4b0d7dd5 Mon Sep 17 00:00:00 2001 From: Zane Hitchcox Date: Mon, 1 Oct 2018 10:16:59 -0400 Subject: [PATCH 26/26] Don't delete fake terminal --- demo/client.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/demo/client.ts b/demo/client.ts index 91cebf07..4de78999 100644 --- a/demo/client.ts +++ b/demo/client.ts @@ -149,7 +149,6 @@ function runRealTerminal(): void { term._initialized = true; } -// TODO: Maybe fake terminal should be removed? Not sure it's useful anymore function runFakeTerminal(): void { if (term._initialized) { return;