From 6225cc2b9c95c730ec37caf6a8bc670a13cb3fce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Tue, 13 Sep 2022 14:37:29 +0200 Subject: [PATCH 01/10] optimize certain resize conditions --- src/common/buffer/BufferLine.ts | 38 ++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/src/common/buffer/BufferLine.ts b/src/common/buffer/BufferLine.ts index 43e89839..ec3c22e5 100644 --- a/src/common/buffer/BufferLine.ts +++ b/src/common/buffer/BufferLine.ts @@ -42,6 +42,8 @@ const w: { startIndex: number } = { startIndex: 0 }; +const EMPTY_DATA = new Uint32Array(0); + /** * Typed array based bufferline implementation. * @@ -64,7 +66,7 @@ export class BufferLine implements IBufferLine { public length: number; constructor(cols: number, fillCellData?: ICellData, public isWrapped: boolean = false) { - this._data = new Uint32Array(cols * CELL_SIZE); + this._data = cols ? new Uint32Array(cols * CELL_SIZE) : EMPTY_DATA; const cell = fillCellData || CellData.fromCharData([0, NULL_CELL_CHAR, NULL_CELL_WIDTH, NULL_CELL_CODE]); for (let i = 0; i < cols; ++i) { this.setCell(i, cell); @@ -339,25 +341,28 @@ export class BufferLine implements IBufferLine { if (cols === this.length) { return; } + const fourByteCells = cols * CELL_SIZE; if (cols > this.length) { - const data = new Uint32Array(cols * CELL_SIZE); - if (this.length) { - if (cols * CELL_SIZE < this._data.length) { - data.set(this._data.subarray(0, cols * CELL_SIZE)); - } else { + if (this._data.buffer.byteLength >= fourByteCells * 4) { + // optimization: avoid alloc and data copy if buffer has enough room + this._data = new Uint32Array(this._data.buffer, 0, fourByteCells); + } else { + // slow path: new alloc and full data copy + const data = new Uint32Array(fourByteCells); + if (this.length && this._data.length <= fourByteCells) { data.set(this._data); } + this._data = data; } - this._data = data; for (let i = this.length; i < cols; ++i) { this.setCell(i, fillCellData); } } else { if (cols) { - const data = new Uint32Array(cols * CELL_SIZE); - data.set(this._data.subarray(0, cols * CELL_SIZE)); - this._data = data; - // Remove any cut off combined data, FIXME: repeat this for extended attrs + // optimization: just shrink the view on existing buffer + // FIXME: register requestIdleCallback() to cleanup memory, if buffer >2x used view + this._data = this._data.subarray(0, fourByteCells); + // Remove any cut off combined data const keys = Object.keys(this._combined); for (let i = 0; i < keys.length; i++) { const key = parseInt(keys[i], 10); @@ -365,9 +370,18 @@ export class BufferLine implements IBufferLine { delete this._combined[key]; } } + // remove any cut off extended attributes + const extKeys = Object.keys(this._extendedAttrs); + for (let i = 0; i < extKeys.length; i++) { + const key = parseInt(extKeys[i], 10); + if (key >= cols) { + delete this._extendedAttrs[key]; + } + } } else { - this._data = new Uint32Array(0); + this._data = EMPTY_DATA; this._combined = {}; + this._extendedAttrs = {}; } } this.length = cols; From e87f70d6d6713586c279c5335921f0dec469dccc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Wed, 28 Sep 2022 11:55:26 +0200 Subject: [PATCH 02/10] removed EMPTY_DATA branch --- src/common/buffer/BufferLine.ts | 65 ++++++++++++++++++++------------- 1 file changed, 39 insertions(+), 26 deletions(-) diff --git a/src/common/buffer/BufferLine.ts b/src/common/buffer/BufferLine.ts index ec3c22e5..dc7ab51c 100644 --- a/src/common/buffer/BufferLine.ts +++ b/src/common/buffer/BufferLine.ts @@ -42,7 +42,8 @@ const w: { startIndex: number } = { startIndex: 0 }; -const EMPTY_DATA = new Uint32Array(0); +/** Factor when to cleanup underlying array buffer after shrinking. */ +const CLEANUP_THRESHOLD = 2; /** * Typed array based bufferline implementation. @@ -66,7 +67,7 @@ export class BufferLine implements IBufferLine { public length: number; constructor(cols: number, fillCellData?: ICellData, public isWrapped: boolean = false) { - this._data = cols ? new Uint32Array(cols * CELL_SIZE) : EMPTY_DATA; + this._data = new Uint32Array(cols * CELL_SIZE); const cell = fillCellData || CellData.fromCharData([0, NULL_CELL_CHAR, NULL_CELL_WIDTH, NULL_CELL_CODE]); for (let i = 0; i < cols; ++i) { this.setCell(i, cell); @@ -337,9 +338,10 @@ export class BufferLine implements IBufferLine { } } - public resize(cols: number, fillCellData: ICellData): void { + public resize(cols: number, fillCellData: ICellData): boolean { + let needsCleanup = false; if (cols === this.length) { - return; + return needsCleanup; } const fourByteCells = cols * CELL_SIZE; if (cols > this.length) { @@ -358,33 +360,44 @@ export class BufferLine implements IBufferLine { this.setCell(i, fillCellData); } } else { - if (cols) { - // optimization: just shrink the view on existing buffer - // FIXME: register requestIdleCallback() to cleanup memory, if buffer >2x used view - this._data = this._data.subarray(0, fourByteCells); - // Remove any cut off combined data - const keys = Object.keys(this._combined); - for (let i = 0; i < keys.length; i++) { - const key = parseInt(keys[i], 10); - if (key >= cols) { - delete this._combined[key]; - } + // optimization: just shrink the view on existing buffer + this._data = this._data.subarray(0, fourByteCells); + needsCleanup = fourByteCells * 4 < this._data.buffer.byteLength * CLEANUP_THRESHOLD; + // Remove any cut off combined data + const keys = Object.keys(this._combined); + for (let i = 0; i < keys.length; i++) { + const key = parseInt(keys[i], 10); + if (key >= cols) { + delete this._combined[key]; } - // remove any cut off extended attributes - const extKeys = Object.keys(this._extendedAttrs); - for (let i = 0; i < extKeys.length; i++) { - const key = parseInt(extKeys[i], 10); - if (key >= cols) { - delete this._extendedAttrs[key]; - } + } + // remove any cut off extended attributes + const extKeys = Object.keys(this._extendedAttrs); + for (let i = 0; i < extKeys.length; i++) { + const key = parseInt(extKeys[i], 10); + if (key >= cols) { + delete this._extendedAttrs[key]; } - } else { - this._data = EMPTY_DATA; - this._combined = {}; - this._extendedAttrs = {}; } } this.length = cols; + return needsCleanup; + } + + /** + * Cleanup underlying array buffer. + * A cleanup will be triggered if the array buffer exceeds the actual used + * memory by a factor of CLEANUP_THRESHOLD. + * Returns 0 or 1 indicating whether a cleanup happened. + */ + public cleanupBuffer(): number { + if (this._data.length * 4 < this._data.buffer.byteLength * CLEANUP_THRESHOLD) { + const data = new Uint32Array(this._data.length); + data.set(this._data); + this._data = data; + return 1; + } + return 0; } /** fill a line with fillCharData */ From 20c1f1a5f72233ba542e12d4dea0560ab69eff90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Wed, 28 Sep 2022 12:14:09 +0200 Subject: [PATCH 03/10] fix resize edge cases: always report cleanup state --- src/common/Types.d.ts | 2 +- src/common/buffer/BufferLine.ts | 13 +++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/common/Types.d.ts b/src/common/Types.d.ts index d44bb197..bbf00f1b 100644 --- a/src/common/Types.d.ts +++ b/src/common/Types.d.ts @@ -205,7 +205,7 @@ export interface IBufferLine { insertCells(pos: number, n: number, ch: ICellData, eraseAttr?: IAttributeData): void; deleteCells(pos: number, n: number, fill: ICellData, eraseAttr?: IAttributeData): void; replaceCells(start: number, end: number, fill: ICellData, eraseAttr?: IAttributeData, respectProtect?: boolean): void; - resize(cols: number, fill: ICellData): void; + resize(cols: number, fill: ICellData): boolean; fill(fillCellData: ICellData, respectProtect?: boolean): void; copyFrom(line: IBufferLine): void; clone(): IBufferLine; diff --git a/src/common/buffer/BufferLine.ts b/src/common/buffer/BufferLine.ts index dc7ab51c..f6279f9d 100644 --- a/src/common/buffer/BufferLine.ts +++ b/src/common/buffer/BufferLine.ts @@ -338,10 +338,16 @@ export class BufferLine implements IBufferLine { } } + /** + * Resize BufferLine to `cols` filling excess cells with `fillCellData`. + * The underlying array buffer will not change if there is still enough space + * to hold the new buffer line data. + * Returns a boolean indicating, whether a `cleanBuffer` call would free + * excess memory (after shrinking > CLEANUP_THRESHOLD). + */ public resize(cols: number, fillCellData: ICellData): boolean { - let needsCleanup = false; if (cols === this.length) { - return needsCleanup; + return this._data.length * 4 < this._data.buffer.byteLength * CLEANUP_THRESHOLD; } const fourByteCells = cols * CELL_SIZE; if (cols > this.length) { @@ -362,7 +368,6 @@ export class BufferLine implements IBufferLine { } else { // optimization: just shrink the view on existing buffer this._data = this._data.subarray(0, fourByteCells); - needsCleanup = fourByteCells * 4 < this._data.buffer.byteLength * CLEANUP_THRESHOLD; // Remove any cut off combined data const keys = Object.keys(this._combined); for (let i = 0; i < keys.length; i++) { @@ -381,7 +386,7 @@ export class BufferLine implements IBufferLine { } } this.length = cols; - return needsCleanup; + return fourByteCells * 4 < this._data.buffer.byteLength * CLEANUP_THRESHOLD; } /** From 39eea8ed88ea671d716184c514652990e840877d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Wed, 28 Sep 2022 13:15:34 +0200 Subject: [PATCH 04/10] remove useless condition --- src/common/buffer/BufferLine.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/common/buffer/BufferLine.ts b/src/common/buffer/BufferLine.ts index f6279f9d..4483fa7a 100644 --- a/src/common/buffer/BufferLine.ts +++ b/src/common/buffer/BufferLine.ts @@ -357,9 +357,7 @@ export class BufferLine implements IBufferLine { } else { // slow path: new alloc and full data copy const data = new Uint32Array(fourByteCells); - if (this.length && this._data.length <= fourByteCells) { - data.set(this._data); - } + data.set(this._data); this._data = data; } for (let i = this.length; i < cols; ++i) { From 7bba07e24ee5bcee711cc2a0c485420190663287 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Wed, 28 Sep 2022 13:18:31 +0200 Subject: [PATCH 05/10] rename fourByteCells to uint32Cells --- src/common/buffer/BufferLine.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/common/buffer/BufferLine.ts b/src/common/buffer/BufferLine.ts index 4483fa7a..2805f3c8 100644 --- a/src/common/buffer/BufferLine.ts +++ b/src/common/buffer/BufferLine.ts @@ -349,14 +349,14 @@ export class BufferLine implements IBufferLine { if (cols === this.length) { return this._data.length * 4 < this._data.buffer.byteLength * CLEANUP_THRESHOLD; } - const fourByteCells = cols * CELL_SIZE; + const uint32Cells = cols * CELL_SIZE; if (cols > this.length) { - if (this._data.buffer.byteLength >= fourByteCells * 4) { + if (this._data.buffer.byteLength >= uint32Cells * 4) { // optimization: avoid alloc and data copy if buffer has enough room - this._data = new Uint32Array(this._data.buffer, 0, fourByteCells); + this._data = new Uint32Array(this._data.buffer, 0, uint32Cells); } else { // slow path: new alloc and full data copy - const data = new Uint32Array(fourByteCells); + const data = new Uint32Array(uint32Cells); data.set(this._data); this._data = data; } @@ -365,7 +365,7 @@ export class BufferLine implements IBufferLine { } } else { // optimization: just shrink the view on existing buffer - this._data = this._data.subarray(0, fourByteCells); + this._data = this._data.subarray(0, uint32Cells); // Remove any cut off combined data const keys = Object.keys(this._combined); for (let i = 0; i < keys.length; i++) { @@ -384,7 +384,7 @@ export class BufferLine implements IBufferLine { } } this.length = cols; - return fourByteCells * 4 < this._data.buffer.byteLength * CLEANUP_THRESHOLD; + return uint32Cells * 4 < this._data.buffer.byteLength * CLEANUP_THRESHOLD; } /** From 785b932d75912fb973df6f5b870b384e09aea90d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Wed, 28 Sep 2022 15:57:58 +0200 Subject: [PATCH 06/10] lazy cleanup on buffer --- src/common/Types.d.ts | 1 + src/common/buffer/Buffer.ts | 29 +++++++++++++++++++++++++++-- src/common/buffer/BufferLine.ts | 8 ++++---- 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/common/Types.d.ts b/src/common/Types.d.ts index bbf00f1b..c4c470ad 100644 --- a/src/common/Types.d.ts +++ b/src/common/Types.d.ts @@ -206,6 +206,7 @@ export interface IBufferLine { deleteCells(pos: number, n: number, fill: ICellData, eraseAttr?: IAttributeData): void; replaceCells(start: number, end: number, fill: ICellData, eraseAttr?: IAttributeData, respectProtect?: boolean): void; resize(cols: number, fill: ICellData): boolean; + cleanupMemory(): number; fill(fillCellData: ICellData, respectProtect?: boolean): void; copyFrom(line: IBufferLine): void; clone(): IBufferLine; diff --git a/src/common/buffer/Buffer.ts b/src/common/buffer/Buffer.ts index c8b0d1b2..83866304 100644 --- a/src/common/buffer/Buffer.ts +++ b/src/common/buffer/Buffer.ts @@ -14,6 +14,7 @@ import { Marker } from 'common/buffer/Marker'; import { IOptionsService, IBufferService } from 'common/services/Services'; import { DEFAULT_CHARSET } from 'common/data/Charsets'; import { ExtendedAttrs } from 'common/buffer/AttributeData'; +import { DebouncedIdleTask } from 'common/TaskQueue'; export const MAX_BUFFER_SIZE = 4294967295; // 2^32 - 1 @@ -151,6 +152,9 @@ export class Buffer implements IBuffer { // store reference to null cell with default attrs const nullCell = this.getNullCell(DEFAULT_ATTR_DATA); + // defer memory cleanup of bufferlines + let needsCleanup = 0; + // Increase max length if needed before adjustments to allow space to fill // as required. const newMaxLength = this._getCorrectBufferLength(newRows); @@ -164,7 +168,7 @@ export class Buffer implements IBuffer { // Deal with columns increasing (reducing needs to happen after reflow) if (this._cols < newCols) { for (let i = 0; i < this.lines.length; i++) { - this.lines.get(i)!.resize(newCols, nullCell); + needsCleanup |= +this.lines.get(i)!.resize(newCols, nullCell); } } @@ -243,13 +247,34 @@ export class Buffer implements IBuffer { // Trim the end of the line off if cols shrunk if (this._cols > newCols) { for (let i = 0; i < this.lines.length; i++) { - this.lines.get(i)!.resize(newCols, nullCell); + needsCleanup |= +this.lines.get(i)!.resize(newCols, nullCell); } } } this._cols = newCols; this._rows = newRows; + + if (needsCleanup) { + this._memoryCleanupTask.set(() => this._cleanupMemory()); + } else { + // FIXME: DebouncedIdleTask has no clear method? + this._memoryCleanupTask.set(() => {}); + } + } + + private _memoryCleanupTask: DebouncedIdleTask = new DebouncedIdleTask(); + + private _cleanupMemory(): void { + let counted = 0; + for (let i = 0; i < this.lines.length; i++) { + counted += this.lines.get(i)!.cleanupMemory(); + // throttle to 5k lines + if (counted > 5000) { + this._memoryCleanupTask.set(() => this._cleanupMemory()); + break; + } + } } private get _isReflowEnabled(): boolean { diff --git a/src/common/buffer/BufferLine.ts b/src/common/buffer/BufferLine.ts index 2805f3c8..2e4fbca9 100644 --- a/src/common/buffer/BufferLine.ts +++ b/src/common/buffer/BufferLine.ts @@ -347,7 +347,7 @@ export class BufferLine implements IBufferLine { */ public resize(cols: number, fillCellData: ICellData): boolean { if (cols === this.length) { - return this._data.length * 4 < this._data.buffer.byteLength * CLEANUP_THRESHOLD; + return this._data.length * 4 * CLEANUP_THRESHOLD < this._data.buffer.byteLength; } const uint32Cells = cols * CELL_SIZE; if (cols > this.length) { @@ -384,7 +384,7 @@ export class BufferLine implements IBufferLine { } } this.length = cols; - return uint32Cells * 4 < this._data.buffer.byteLength * CLEANUP_THRESHOLD; + return uint32Cells * 4 * CLEANUP_THRESHOLD < this._data.buffer.byteLength; } /** @@ -393,8 +393,8 @@ export class BufferLine implements IBufferLine { * memory by a factor of CLEANUP_THRESHOLD. * Returns 0 or 1 indicating whether a cleanup happened. */ - public cleanupBuffer(): number { - if (this._data.length * 4 < this._data.buffer.byteLength * CLEANUP_THRESHOLD) { + public cleanupMemory(): number { + if (this._data.length * 4 * CLEANUP_THRESHOLD < this._data.buffer.byteLength) { const data = new Uint32Array(this._data.length); data.set(this._data); this._data = data; From 1f5159febe263698042c8faf95009307df46217a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Sat, 3 Dec 2022 16:49:26 +0100 Subject: [PATCH 07/10] use IdleTaskQueue & Date.now; some testcases --- src/common/TaskQueue.ts | 30 +++++++++++++++++--------- src/common/buffer/Buffer.test.ts | 30 ++++++++++++++++++++++++++ src/common/buffer/Buffer.ts | 37 +++++++++++++++++++------------- 3 files changed, 72 insertions(+), 25 deletions(-) diff --git a/src/common/TaskQueue.ts b/src/common/TaskQueue.ts index 94c5c53b..021bb7ce 100644 --- a/src/common/TaskQueue.ts +++ b/src/common/TaskQueue.ts @@ -8,8 +8,11 @@ import { isNode } from 'common/Platform'; interface ITaskQueue { /** * Adds a task to the queue which will run in a future idle callback. + * To avoid perceivable stalls on the mainthread, tasks with heavy workload + * should split their work into smaller pieces and return `true` to get + * called again until the work is done (on falsy return value). */ - enqueue(task: () => void): void; + enqueue(task: () => boolean | void): void; /** * Flushes the queue, running all remaining tasks synchronously. @@ -28,21 +31,23 @@ interface ITaskDeadline { type CallbackWithDeadline = (deadline: ITaskDeadline) => void; abstract class TaskQueue implements ITaskQueue { - private _tasks: (() => void)[] = []; + private _tasks: (() => boolean | void)[] = []; private _idleCallback?: number; private _i = 0; protected abstract _requestCallback(callback: CallbackWithDeadline): number; protected abstract _cancelCallback(identifier: number): void; - public enqueue(task: () => void): void { + public enqueue(task: () => boolean | void): void { this._tasks.push(task); this._start(); } public flush(): void { while (this._i < this._tasks.length) { - this._tasks[this._i++](); + if (!this._tasks[this._i]()) { + this._i++; + } } this.clear(); } @@ -67,9 +72,14 @@ abstract class TaskQueue implements ITaskQueue { let taskDuration = 0; let longestTask = 0; while (this._i < this._tasks.length) { - taskDuration = performance.now(); - this._tasks[this._i++](); - taskDuration = performance.now() - taskDuration; + taskDuration = Date.now(); + if (!this._tasks[this._i]()) { + this._i++; + } + // other than performance.now, Date.now might not be stable (changes on wall clock changes), + // this is not an issue here as a clock change during a short running task is very unlikely + // in case it still happened and leads to negative duration, simply assume 1 msec + taskDuration = Math.max(1, Date.now() - taskDuration); longestTask = Math.max(taskDuration, longestTask); // Guess the following task will take a similar time to the longest task in this batch, allow // additional room to try avoid exceeding the deadline @@ -97,9 +107,9 @@ export class PriorityTaskQueue extends TaskQueue { } private _createDeadline(duration: number): ITaskDeadline { - const end = performance.now() + duration; + const end = Date.now() + duration; return { - timeRemaining: () => Math.max(0, end - performance.now()) + timeRemaining: () => Math.max(0, end - Date.now()) }; } } @@ -136,7 +146,7 @@ export class DebouncedIdleTask { this._queue = new IdleTaskQueue(); } - public set(task: () => void): void { + public set(task: () => boolean | void): void { this._queue.clear(); this._queue.enqueue(task); } diff --git a/src/common/buffer/Buffer.test.ts b/src/common/buffer/Buffer.test.ts index e5ea7f5e..e854ce34 100644 --- a/src/common/buffer/Buffer.test.ts +++ b/src/common/buffer/Buffer.test.ts @@ -9,6 +9,7 @@ import { CircularList } from 'common/CircularList'; import { MockOptionsService, MockBufferService } from 'common/TestUtils.test'; import { BufferLine, DEFAULT_ATTR_DATA } from 'common/buffer/BufferLine'; import { CellData } from 'common/buffer/CellData'; +import { ExtendedAttrs } from 'common/buffer/AttributeData'; const INIT_COLS = 80; const INIT_ROWS = 24; @@ -1177,4 +1178,33 @@ describe('Buffer', () => { assert.equal(str3, '😁a'); }); }); + + describe('memory cleanup after shrinking', () => { + it('should realign memory from idle task execution', async () => { + buffer.fillViewportRows(); + + // shrink more than 2 times to trigger lazy memory cleanup + buffer.resize(INIT_COLS / 2 - 1, INIT_ROWS); + + // sync + for (let i = 0; i < INIT_ROWS; i++) { + const line = buffer.lines.get(i)!; + // line memory is still at old size from initialization + assert.equal((line as any)._data.buffer.byteLength, INIT_COLS * 3 * 4); + // array.length and .length get immediately adjusted + assert.equal((line as any)._data.length, (INIT_COLS / 2 - 1) * 3); + assert.equal(line.length, INIT_COLS / 2 - 1); + } + + // wait for a bit to give IdleTaskQueue a chance to kick in + // and finish memory cleaning + await new Promise(r => setTimeout(r, 100)); + + // cleanup should have realigned memory with exact bytelength + for (let i = 0; i < INIT_ROWS; i++) { + const line = buffer.lines.get(i)!; + assert.equal((line as any)._data.buffer.byteLength, (INIT_COLS / 2 - 1) * 3 * 4); + } + }); + }); }); diff --git a/src/common/buffer/Buffer.ts b/src/common/buffer/Buffer.ts index 5c67be56..4890e7c2 100644 --- a/src/common/buffer/Buffer.ts +++ b/src/common/buffer/Buffer.ts @@ -14,7 +14,7 @@ import { Marker } from 'common/buffer/Marker'; import { IOptionsService, IBufferService } from 'common/services/Services'; import { DEFAULT_CHARSET } from 'common/data/Charsets'; import { ExtendedAttrs } from 'common/buffer/AttributeData'; -import { DebouncedIdleTask } from 'common/TaskQueue'; +import { DebouncedIdleTask, IdleTaskQueue } from 'common/TaskQueue'; export const MAX_BUFFER_SIZE = 4294967295; // 2^32 - 1 @@ -151,8 +151,8 @@ export class Buffer implements IBuffer { // store reference to null cell with default attrs const nullCell = this.getNullCell(DEFAULT_ATTR_DATA); - // defer memory cleanup of bufferlines - let needsCleanup = 0; + // count bufferlines with overly big memory to be cleaned afterwards + let dirtyMemoryLines = 0; // Increase max length if needed before adjustments to allow space to fill // as required. @@ -168,7 +168,7 @@ export class Buffer implements IBuffer { if (this._cols < newCols) { for (let i = 0; i < this.lines.length; i++) { // +boolean for fast 0 or 1 conversion - needsCleanup |= +this.lines.get(i)!.resize(newCols, nullCell); + dirtyMemoryLines += +this.lines.get(i)!.resize(newCols, nullCell); } } @@ -248,7 +248,7 @@ export class Buffer implements IBuffer { if (this._cols > newCols) { for (let i = 0; i < this.lines.length; i++) { // +boolean for fast 0 or 1 conversion - needsCleanup |= +this.lines.get(i)!.resize(newCols, nullCell); + dirtyMemoryLines += +this.lines.get(i)!.resize(newCols, nullCell); } } } @@ -256,26 +256,33 @@ export class Buffer implements IBuffer { this._cols = newCols; this._rows = newRows; - if (needsCleanup) { - this._memoryCleanupTask.set(() => this._cleanupMemory()); - } else { - // FIXME: DebouncedIdleTask has no clear method? - this._memoryCleanupTask.set(() => {}); + this._memoryCleanupQueue.clear(); + // schedule memory cleanup only, if more than 10% of the lines are affected + if (dirtyMemoryLines > 0.1 * this.lines.length) { + this._memoryCleanupQueue.enqueue(() => this._batchedMemoryCleanup()); } } - private _memoryCleanupTask: DebouncedIdleTask = new DebouncedIdleTask(); + private _memoryCleanupQueue = new IdleTaskQueue(); - private _cleanupMemory(): void { + private _batchedMemoryCleanup(): boolean { let counted = 0; for (let i = 0; i < this.lines.length; i++) { counted += this.lines.get(i)!.cleanupMemory(); - // throttle to 5k lines + // throttle to 5k lines at once and + // return true to indicate, that the task is not finished yet if (counted > 5000) { - this._memoryCleanupTask.set(() => this._cleanupMemory()); - break; + return true; } } + return false; + } + + private _forceMemoryCleanup(): void { + this._memoryCleanupQueue.clear(); + for (let i = 0; i < this.lines.length; i++) { + this.lines.get(i)!.cleanupMemory(); + } } private get _isReflowEnabled(): boolean { From 4084dc3949d4f41be23b411e22f962198079031c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Mon, 5 Dec 2022 19:10:55 +0100 Subject: [PATCH 08/10] remove _forceMemoryCleanup --- src/common/buffer/Buffer.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/common/buffer/Buffer.ts b/src/common/buffer/Buffer.ts index 4890e7c2..5ecf19bb 100644 --- a/src/common/buffer/Buffer.ts +++ b/src/common/buffer/Buffer.ts @@ -278,13 +278,6 @@ export class Buffer implements IBuffer { return false; } - private _forceMemoryCleanup(): void { - this._memoryCleanupQueue.clear(); - for (let i = 0; i < this.lines.length; i++) { - this.lines.get(i)!.cleanupMemory(); - } - } - private get _isReflowEnabled(): boolean { return this._hasScrollback && !this._optionsService.rawOptions.windowsMode; } From 309ba47483840a616931122877c4e5d93d56aab9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Mon, 5 Dec 2022 20:03:25 +0100 Subject: [PATCH 09/10] change batch size to 500 --- src/common/buffer/Buffer.ts | 4 ++-- src/common/buffer/BufferLine.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/common/buffer/Buffer.ts b/src/common/buffer/Buffer.ts index 5ecf19bb..4f02e36d 100644 --- a/src/common/buffer/Buffer.ts +++ b/src/common/buffer/Buffer.ts @@ -269,9 +269,9 @@ export class Buffer implements IBuffer { let counted = 0; for (let i = 0; i < this.lines.length; i++) { counted += this.lines.get(i)!.cleanupMemory(); - // throttle to 5k lines at once and + // throttle to 500 lines at once and // return true to indicate, that the task is not finished yet - if (counted > 5000) { + if (counted > 500) { return true; } } diff --git a/src/common/buffer/BufferLine.ts b/src/common/buffer/BufferLine.ts index ac4ea768..d5f43844 100644 --- a/src/common/buffer/BufferLine.ts +++ b/src/common/buffer/BufferLine.ts @@ -340,8 +340,8 @@ export class BufferLine implements IBufferLine { * Resize BufferLine to `cols` filling excess cells with `fillCellData`. * The underlying array buffer will not change if there is still enough space * to hold the new buffer line data. - * Returns a boolean indicating, whether a `cleanBuffer` call would free - * excess memory (after shrinking > CLEANUP_THRESHOLD). + * Returns a boolean indicating, whether a `cleanupMemory` call would free + * excess memory (true after shrinking > CLEANUP_THRESHOLD). */ public resize(cols: number, fillCellData: ICellData): boolean { if (cols === this.length) { From a562f33d530801b8014141971b025c0cc15fa424 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Wed, 7 Dec 2022 19:43:56 +0100 Subject: [PATCH 10/10] remove nonsense exp runtime of rescans --- src/common/buffer/Buffer.test.ts | 2 +- src/common/buffer/Buffer.ts | 23 +++++++++++++++++------ 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/common/buffer/Buffer.test.ts b/src/common/buffer/Buffer.test.ts index e854ce34..39cffb49 100644 --- a/src/common/buffer/Buffer.test.ts +++ b/src/common/buffer/Buffer.test.ts @@ -1198,7 +1198,7 @@ describe('Buffer', () => { // wait for a bit to give IdleTaskQueue a chance to kick in // and finish memory cleaning - await new Promise(r => setTimeout(r, 100)); + await new Promise(r => setTimeout(r, 30)); // cleanup should have realigned memory with exact bytelength for (let i = 0; i < INIT_ROWS; i++) { diff --git a/src/common/buffer/Buffer.ts b/src/common/buffer/Buffer.ts index 4f02e36d..b935b2a3 100644 --- a/src/common/buffer/Buffer.ts +++ b/src/common/buffer/Buffer.ts @@ -259,23 +259,34 @@ export class Buffer implements IBuffer { this._memoryCleanupQueue.clear(); // schedule memory cleanup only, if more than 10% of the lines are affected if (dirtyMemoryLines > 0.1 * this.lines.length) { + this._memoryCleanupPosition = 0; this._memoryCleanupQueue.enqueue(() => this._batchedMemoryCleanup()); } } private _memoryCleanupQueue = new IdleTaskQueue(); + private _memoryCleanupPosition = 0; private _batchedMemoryCleanup(): boolean { + let normalRun = true; + if (this._memoryCleanupPosition >= this.lines.length) { + // cleanup made it once through all lines, thus rescan in loop below to also catch shifted lines, + // which should finish rather quick if there are no more cleanups pending + this._memoryCleanupPosition = 0; + normalRun = false; + } let counted = 0; - for (let i = 0; i < this.lines.length; i++) { - counted += this.lines.get(i)!.cleanupMemory(); - // throttle to 500 lines at once and - // return true to indicate, that the task is not finished yet - if (counted > 500) { + while (this._memoryCleanupPosition < this.lines.length) { + counted += this.lines.get(this._memoryCleanupPosition++)!.cleanupMemory(); + // cleanup max 100 lines per batch + if (counted > 100) { return true; } } - return false; + // normal runs always need another rescan afterwards + // if we made it here with normalRun=false, we are in a final run + // and can end the cleanup task for sure + return normalRun; } private get _isReflowEnabled(): boolean {