From cc005a274dc440c40cf985fe1ebfa184e2f0f517 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Sat, 27 Feb 2021 20:18:41 +0100 Subject: [PATCH] multiple changes: - reorder exception throwing to be sync to band position - document WriteBuffer._innerWrite - remove any declarations - remove conditional assigments - use faster return value branching in all parsers - remove dead code in benchmark --- src/common/InputHandler.ts | 4 +- src/common/input/WriteBuffer.ts | 57 +++++++++++++++++++---- src/common/parser/DcsParser.ts | 28 +++++------ src/common/parser/EscapeSequenceParser.ts | 55 ++++++++++++---------- src/common/parser/OscParser.ts | 26 ++++++----- test/benchmark/Terminal.benchmark.ts | 22 --------- 6 files changed, 111 insertions(+), 81 deletions(-) diff --git a/src/common/InputHandler.ts b/src/common/InputHandler.ts index 08f04d9e..f663ded7 100644 --- a/src/common/InputHandler.ts +++ b/src/common/InputHandler.ts @@ -488,8 +488,8 @@ export class InputHandler extends Disposable implements IInputHandler { * execution stopped at async handler, stack saved, continue with * same chunk and the promise resolve value as `promiseResult` until the method returns `undefined` * - * Note: Never call this directly for a running terminal instance in production. - * Always use `Terminal.write`, which provides in-band blocking and correct exection order. + * Note: This method should only be called by `Terminal.write` to ensure correct execution order and + * proper continuation of async parser handlers. */ public parse(data: string | Uint8Array, promiseResult?: boolean): void | Promise { let result: void | Promise; diff --git a/src/common/input/WriteBuffer.ts b/src/common/input/WriteBuffer.ts index c6178024..a2c70004 100644 --- a/src/common/input/WriteBuffer.ts +++ b/src/common/input/WriteBuffer.ts @@ -31,6 +31,12 @@ const WRITE_TIMEOUT_MS = 12; */ const WRITE_BUFFER_LENGTH_THRESHOLD = 50; +// queueMicrotask polyfill for nodejs < v11 +const qmt: (cb: () => void) => void = (typeof queueMicrotask === 'undefined') + ? (cb: () => void) => { Promise.resolve().then(cb); } + : queueMicrotask; + + export class WriteBuffer { private _writeBuffer: (string | Uint8Array)[] = []; private _callbacks: ((() => void) | undefined)[] = []; @@ -39,7 +45,9 @@ export class WriteBuffer { constructor(private _action: (data: string | Uint8Array, promiseResult?: boolean) => void | Promise) { } - // FIXME: does not work that way anymore with async handlers!!! + /** + * @deprecated Unreliable, to be removed soon. + */ public writeSync(data: string | Uint8Array): void { // force sync processing on pending data chunks to avoid in-band data scrambling // does the same as innerWrite but without event loop @@ -77,13 +85,40 @@ export class WriteBuffer { this._callbacks.push(callback); } - protected _innerWrite(d: number = 0, promiseResult: boolean = true): void { - let result: void | Promise; - const startTime = d || Date.now(); + /** + * Inner write call, that enters the sliced chunk processing by timing. + * + * `lastTime` indicates, when the last _innerWrite call had started. + * It is used to aggregate async handler execution under a timeout constraint + * effectively lowering the redrawing needs, schematically: + * + * macroTask _innerWrite: + * if (Date.now() - (lastTime | 0) < WRITE_TIMEOUT_MS): + * schedule microTask _innerWrite(lastTime) + * else: + * schedule macroTask _innerWrite(0) + * + * overall execution order on task queues: + * + * macrotasks: [...] --> _innerWrite(0) --> [...] --> screenUpdate --> [...] + * m t: | + * i a: [...] + * c s: | + * r k: while < timeout: + * o s: _innerWrite(timeout) + * + * `promiseResult` depicts the promise resolve value of an async handler. + * This value gets carried forward through all saved stack states of the + * paused parser for proper continuation. + * + * Note, for pure sync code `lastTime` and `promiseResult` have no meaning. + */ + protected _innerWrite(lastTime: number = 0, promiseResult: boolean = true): void { + const startTime = lastTime || Date.now(); while (this._writeBuffer.length > this._bufferOffset) { const data = this._writeBuffer[this._bufferOffset]; - - if (result = this._action(data, promiseResult)) { + const result = this._action(data, promiseResult); + if (result) { /** * If we get a promise as return value, we re-schedule the continuation * as thenable on the promise and exit right away. @@ -98,7 +133,6 @@ export class WriteBuffer { * * Exceptions on async handlers will be logged to console async, but do not interrupt * the input processing (continues with next handler at the current input position). - * FIXME: No clear exception handling rules for sync handlers yet (will exit whole processing?). */ /** @@ -127,7 +161,14 @@ export class WriteBuffer { // ? r => setTimeout(() => this._innerWrite(0, r)) // : r => this._innerWrite(startTime, r); - result.then(continuation, err => { setTimeout(() => { throw err; }); continuation(true); }); + // Handle exceptions synchronously to current band position, idea: + // 1. spawn a single microtask which we allow to throw hard + // 2. spawn a promise immediately resolving to `true` + // (executed on the same queue, thus properly aligned before continuation happens) + result.catch(err => { + qmt(() => {throw err;}); + return Promise.resolve(true); + }).then(continuation); return; } diff --git a/src/common/parser/DcsParser.ts b/src/common/parser/DcsParser.ts index 5389b828..6b5ddd6e 100644 --- a/src/common/parser/DcsParser.ts +++ b/src/common/parser/DcsParser.ts @@ -88,11 +88,11 @@ export class DcsParser implements IDcsParser { loopPosition: 0, fallThrough: false }; - public unhook(success: boolean, promiseResult?: boolean): void | Promise { + public unhook(success: boolean, promiseResult: boolean = true): void | Promise { if (!this._active.length) { this._handlerFb(this._ident, 'UNHOOK', success); } else { - let handlerResult: any = false; + let handlerResult: boolean | Promise = false; let j = this._active.length - 1; let fallThrough = false; if (this._stack.paused) { @@ -103,21 +103,22 @@ export class DcsParser implements IDcsParser { } if (!fallThrough && handlerResult === false) { for (; j >= 0; j--) { - if ((handlerResult = this._active[j].unhook(success)) !== false) { - if (handlerResult instanceof Promise) { - this._stack.paused = true; - this._stack.loopPosition = j; - this._stack.fallThrough = false; - return handlerResult; - } + handlerResult = this._active[j].unhook(success); + if (handlerResult === true) { break; + } else if (handlerResult instanceof Promise) { + this._stack.paused = true; + this._stack.loopPosition = j; + this._stack.fallThrough = false; + return handlerResult; } } j--; } // cleanup left over handlers (fallThrough for async) for (; j >= 0; j--) { - if ((handlerResult = this._active[j].unhook(false)) instanceof Promise) { + handlerResult = this._active[j].unhook(false); + if (handlerResult instanceof Promise) { this._stack.paused = true; this._stack.loopPosition = j; this._stack.fallThrough = true; @@ -171,10 +172,11 @@ export class DcsHandler implements IDcsHandler { if (this._hitLimit) { ret = false; } else if (success) { - if ((ret = this._handler(this._data, this._params)) instanceof Promise) { - // FIXME: should this be behind a catch rule? + ret = this._handler(this._data, this._params); + if (ret instanceof Promise) { + // need to hold data and params until `ret` got resolved + // dont care for errors, data will be freed anyway on next start return ret.then(res => { - // cleanup handler state late this._params = EMPTY_PARAMS; this._data = ''; this._hitLimit = false; diff --git a/src/common/parser/EscapeSequenceParser.ts b/src/common/parser/EscapeSequenceParser.ts index 9ed3e462..c1d80c5c 100644 --- a/src/common/parser/EscapeSequenceParser.ts +++ b/src/common/parser/EscapeSequenceParser.ts @@ -468,7 +468,8 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP handlers: ResumableHandlersType, handlerPos: number, transition: number, - chunkPos: number): void { + chunkPos: number + ): void { this._parseStack.state = state; this._parseStack.handlers = handlers; this._parseStack.handlerPos = handlerPos; @@ -521,12 +522,12 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP let code = 0; let transition = 0; let start = 0; - let handlerResult: any; + let handlerResult: void | boolean | Promise; // resume from async handler if (this._parseStack.state) { // allow sync parser reset even in continuation mode - // Note: can be used to recover parser from improper continuation error above + // Note: can be used to recover parser from improper continuation error below if (this._parseStack.state === ParserStackType.RESET) { this._parseStack.state = ParserStackType.NONE; start = this._parseStack.chunkPos + 1; // continue with next codepoint in GROUND @@ -534,8 +535,8 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP if (promiseResult === undefined || this._parseStack.state === ParserStackType.FAIL) { /** * Reject further parsing on improper continuation after pausing. - * This is a really bad condition with screwed up execution order and messed up terminal state, - * therefore we exit hard with an exception and reject any further parsing. + * This is a really bad condition with screwed up execution order and prolly messed up + * terminal state, therefore we exit hard with an exception and reject any further parsing. * * Note: With `Terminal.write` usage this exception should never occur, as the top level * calls are guaranteed to handle async conditions properly. If you ever encounter this @@ -543,8 +544,8 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP * `InputHandler.parse` or `EscapeSequenceParser.parse` synchronously without waiting for * continuation of a running async handler. * - * Its possible to get rid of this error condition by calling `reset`, but dont rely on that, - * as the pending async handler might mess up the terminal even further. Instead fix the faulty + * It is possible to get rid of this error by calling `reset`. But dont rely on that, + * as the pending async handler still might mess up the terminal later. Instead fix the faulty * async handling, so this error will not be thrown anymore. */ this._parseStack.state = ParserStackType.FAIL; @@ -554,20 +555,18 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP // we have to resume the old handler loop if: // - return value of the promise was `false` // - handlers are not exhausted yet - // FIXME: removing handlers from within a handler of the same sequence - // is not supported atm (also true for sync handlers)!! const handlers = this._parseStack.handlers; let handlerPos = this._parseStack.handlerPos - 1; switch (this._parseStack.state) { case ParserStackType.CSI: if (promiseResult === false && handlerPos > -1) { for (; handlerPos >= 0; handlerPos--) { - if ((handlerResult = (handlers as CsiHandlerType[])[handlerPos](this._params)) !== false) { - if (handlerResult instanceof Promise) { - this._parseStack.handlerPos = handlerPos; - return handlerResult; - } + handlerResult = (handlers as CsiHandlerType[])[handlerPos](this._params); + if (handlerResult === true) { break; + } else if (handlerResult instanceof Promise) { + this._parseStack.handlerPos = handlerPos; + return handlerResult; } } } @@ -576,12 +575,12 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP case ParserStackType.ESC: if (promiseResult === false && handlerPos > -1) { for (; handlerPos >= 0; handlerPos--) { - if ((handlerResult = (handlers as EscHandlerType[])[handlerPos]()) !== false) { - if (handlerResult instanceof Promise) { - this._parseStack.handlerPos = handlerPos; - return handlerResult; - } + handlerResult = (handlers as EscHandlerType[])[handlerPos](); + if (handlerResult === true) { break; + } else if (handlerResult instanceof Promise) { + this._parseStack.handlerPos = handlerPos; + return handlerResult; } } } @@ -589,7 +588,8 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP break; case ParserStackType.DCS: code = data[this._parseStack.chunkPos]; - if (handlerResult = this._dcsParser.unhook(code !== 0x18 && code !== 0x1a, promiseResult)) { + handlerResult = this._dcsParser.unhook(code !== 0x18 && code !== 0x1a, promiseResult); + if (handlerResult) { return handlerResult; } if (code === 0x1b) this._parseStack.transition |= ParserState.ESCAPE; @@ -599,7 +599,8 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP break; case ParserStackType.OSC: code = data[this._parseStack.chunkPos]; - if (handlerResult = this._oscParser.end(code !== 0x18 && code !== 0x1a, promiseResult)) { + handlerResult = this._oscParser.end(code !== 0x18 && code !== 0x1a, promiseResult); + if (handlerResult) { return handlerResult; } if (code === 0x1b) this._parseStack.transition |= ParserState.ESCAPE; @@ -678,7 +679,8 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP for (; j >= 0; j--) { // true means success and to stop bubbling // a promise indicates an async handler that needs to finish before progressing - if ((handlerResult = handlers[j](this._params)) === true) { + handlerResult = handlers[j](this._params); + if (handlerResult === true) { break; } else if (handlerResult instanceof Promise) { this._preserveStack(ParserStackType.CSI, handlers, j, transition, i); @@ -716,7 +718,8 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP for (; jj >= 0; jj--) { // true means success and to stop bubbling // a promise indicates an async handler that needs to finish before progressing - if ((handlerResult = handlersEsc[jj]()) === true) { + handlerResult = handlersEsc[jj](); + if (handlerResult === true) { break; } else if (handlerResult instanceof Promise) { this._preserveStack(ParserStackType.ESC, handlersEsc, jj, transition, i); @@ -748,7 +751,8 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP } break; case ParserAction.DCS_UNHOOK: - if (handlerResult = this._dcsParser.unhook(code !== 0x18 && code !== 0x1a)) { + handlerResult = this._dcsParser.unhook(code !== 0x18 && code !== 0x1a); + if (handlerResult) { this._preserveStack(ParserStackType.DCS, [], 0, transition, i); return handlerResult; } @@ -772,7 +776,8 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP } break; case ParserAction.OSC_END: - if (handlerResult = this._oscParser.end(code !== 0x18 && code !== 0x1a)) { + handlerResult = this._oscParser.end(code !== 0x18 && code !== 0x1a); + if (handlerResult) { this._preserveStack(ParserStackType.OSC, [], 0, transition, i); return handlerResult; } diff --git a/src/common/parser/OscParser.ts b/src/common/parser/OscParser.ts index 1516308c..f043bd82 100644 --- a/src/common/parser/OscParser.ts +++ b/src/common/parser/OscParser.ts @@ -130,7 +130,7 @@ export class OscParser implements IOscParser { * Whether the OSC got aborted or finished normally * is indicated by `success`. */ - public end(success: boolean, promiseResult?: boolean): void | Promise { + public end(success: boolean, promiseResult: boolean = true): void | Promise { if (this._state === OscState.START) { return; } @@ -146,7 +146,7 @@ export class OscParser implements IOscParser { if (!this._active.length) { this._handlerFb(this._id, 'END', success); } else { - let handlerResult: any = false; + let handlerResult: boolean | Promise = false; let j = this._active.length - 1; let fallThrough = false; if (this._stack.paused) { @@ -157,14 +157,14 @@ export class OscParser implements IOscParser { } if (!fallThrough && handlerResult === false) { for (; j >= 0; j--) { - if ((handlerResult = this._active[j].end(success)) !== false) { - if (handlerResult instanceof Promise) { - this._stack.paused = true; - this._stack.loopPosition = j; - this._stack.fallThrough = false; - return handlerResult; - } + handlerResult = this._active[j].end(success); + if (handlerResult === true) { break; + } else if (handlerResult instanceof Promise) { + this._stack.paused = true; + this._stack.loopPosition = j; + this._stack.fallThrough = false; + return handlerResult; } } j--; @@ -173,7 +173,8 @@ export class OscParser implements IOscParser { // we always have to call .end for proper cleanup, // here we use `success` to indicate whether a handler should execute for (; j >= 0; j--) { - if ((handlerResult = this._active[j].end(false)) instanceof Promise) { + handlerResult = this._active[j].end(false); + if (handlerResult instanceof Promise) { this._stack.paused = true; this._stack.loopPosition = j; this._stack.fallThrough = true; @@ -220,7 +221,10 @@ export class OscHandler implements IOscHandler { if (this._hitLimit) { ret = false; } else if (success) { - if ((ret = this._handler(this._data)) instanceof Promise) { + ret = this._handler(this._data); + if (ret instanceof Promise) { + // need to hold data until `ret` got resolved + // dont care for errors, data will be freed anyway on next start return ret.then(res => { this._data = ''; this._hitLimit = false; diff --git a/test/benchmark/Terminal.benchmark.ts b/test/benchmark/Terminal.benchmark.ts index 7d0f5d84..40a3c208 100644 --- a/test/benchmark/Terminal.benchmark.ts +++ b/test/benchmark/Terminal.benchmark.ts @@ -44,28 +44,6 @@ perfContext('Terminal: ls -lR /usr/lib', () => { } }); - // perfContext('write/string/sync', () => { - // let terminal: Terminal; - // before(() => { - // terminal = new Terminal({cols: 80, rows: 25, scrollback: 1000}); - // }); - // new ThroughputRuntimeCase('', () => { - // terminal.writeSync(content); - // return {payloadSize: contentUtf8.length}; - // }, {fork: false}).showAverageThroughput(); - // }); - // - // perfContext('write/Utf8/sync', () => { - // let terminal: Terminal; - // before(() => { - // terminal = new Terminal({cols: 80, rows: 25, scrollback: 1000}); - // }); - // new ThroughputRuntimeCase('', () => { - // terminal.writeSync(content); - // return {payloadSize: contentUtf8.length}; - // }, {fork: false}).showAverageThroughput(); - // }); - perfContext('write/string/async', () => { let terminal: Terminal; before(() => {