new parser #1399

jerch · 2018-04-21T03:22:52Z

WIP - don't merge...

basic transition to ansi complicant parser done
speed gain ~30%

TODO:

tests for glue class ParserTerminal
refactoring with API for hooking custom escape codes and methods
maybe merge ParserTerminal with InputHandler
better decoupling of the higher level stuff

jerch · 2018-04-22T16:02:55Z

This PR is up for a first technical and conceptual review:

About EscapeSequenceParser:

implements the reference from https://vt100.net/emu/dec_ansi_parser as FSM and should therefore support all legacy escape sequences
newer sequences like CSI with parameter element colon notation are not supported atm (were not part of the old DEC machines), can be added with some effort
custom transition maps via parser argument
custom transitions on the fly possible via TransitionTable.add
FSM actions are hardcoded as switch statement in the parse method for performance reasons
hot parse loop avoids memory allocations as much as possible (no string copies)
supports error propagation (back propagation not implemented yet)
fully tested

Interface to xterm.js:

realized by class ParserTerminal implementing the action callbacks needed by EscapeSequenceParser
no magic here - was straight forward to integrate with existing InputHandler methods
good place to allow custom sequence handler hooks
DCS not yet implemented
no tests yet

Results so far:

passes all tests, no awkward behavior during live tests
10% speed gain by switching to the new parser
performance tests revealed InputHandler.addChar as main bottleneck (localizing _terminal.buffer gave 20% performance boost, seems the getters to the buffer implementation are very expensive - Should this be tracked in a new issue?)

There is still alot refactoring needed, several questions popped during coding (also see code comments):

The Parser uses atm the global transition table by default. Should this be copied over to new instances instead to allow hot custom state patches without altering other terminals?
Should all escape sequence types be customizable or only OSC and DCS?
Should ParserTerminal be merged with InputHandler? If so this could be the base for a screen/input decoupled terminal class since it could handle all the escape sequence related stuff.
There are still many references to the _terminal object. Are there any plans yet to decouple this further?
The execute action does not define any method for XON or XOFF (copied over). Is this done by a different layer? Or does xterm.js currently not support XON/XOFF from slave side?
The execute action falls back to addChar atm (again copied over). I think this needs to be revised since it might lead to weird output sometimes of characters that should not be printed at all.
in ESC action: Does xterm.js support single G2/G3 shifts?
in ESC action: I did not understand the code for ESC / - isnt this code here unreachable?
How to deal with custom escape sequence handlers? Registering/deregistering could be done in a typical event like fashion or by a more restrictive coupling to some API methods. Not sure what is better here, if the class would just emit escape sequence events people might get greedy with this and that, also performance will suffer. It would be more versatile though.

This is alot at once, maybe someone finds some time to address some of the questions and basic layout ideas. @Tyriar @parisk @mofux 😄

Tyriar · 2018-04-22T20:48:27Z

src/EscapeSequenceParser.ts

+
+    actionCSI(collected: string, params: number[], flag: string): void {
+        this._terminal.prefix = collected;
+        switch (flag) {


Is the plan is eventually to move all these back to a map? I moved these from a switch to a map to reduce the number of conditionals at the cost of a function pointer for each option.

Also would something like this be better?

interface IEscapeSequenceParser { registerCsiHandler(char: string, callback: (params: x) => y): void; }

Yep, gonna try to find out, how to jump with the lowest costs. I think emitting the charCode from EscapeSequenceParser and placing the function pointers directly into an object will be the fastest (at least in chrome numerical properties avoid the key hashing). As soon as string type properties are involved, a switch is faster until ~20 entries or so.
About the interface - since I am not that good with typescript declarations any more typescripty suggestion is welcome.

What I was getting at with the interface was a different way of thinking about the problem; instead of having ParserTerminal implement actionCSI, what if ParserTerminal/InputHandler called:

parser.registerCsiHandler('@', this._inputHandler.insertChars); parser.registerCsiHandler('A', this._inputHandler.cursorUp);

(as opposed to)

case '@': return this._inputHandler.insertChars(params); case 'A': return this._inputHandler.cursorUp(params);

Yes thats basically the decorator pattern I had in mind for the DCS part, should work for the others as well. 👍

Hmm do we need support for multiple handlers for a single action? This would allow stacking functionality for addons on top of the the basic functionality. Not sure if maintaining an array of function pointers will hurt performance wise.

Maybe starting with support for a single handler is enough for the moment. With the suggested API it wouldn't be complicated to add support for multiple handlers later on if a use-case comes up for it. I'd think that emitting more higher-level events from the input handler (like "linefeed" or "cursorMove") is more appealing for most use-cases.

Tyriar · 2018-04-23T23:06:00Z

src/EscapeSequenceParser.ts

-    inst_P?: (dcs: string) => void;
-    inst_U?: () => void;
-    inst_E?: () => void; // TODO: real signature
+    actionPrint?: (data: string, start: number, end: number) => void;


You should make this mandatory (remove ?), otherwise you would need to check for its existence before calling every time

Kinda obsolete with the new jump tables.

Tyriar · 2018-04-23T23:06:55Z

src/EscapeSequenceParser.ts

+
+// default transition table points to global object
+// Q: Copy table to allow custom sequences w'o changing global object?
+export class EscapeSequenceParser {
    public initialState: number;
    public currentState: number;
    public transitions: TransitionTable;
    public osc: string;
    public params: number[];
    public collected: string;
    public term: any;


Is any just temporary?

Yes, term got removed.

Tyriar · 2018-04-23T23:07:28Z

src/EscapeSequenceParser.ts

-        this.transitions = new TransitionTable(4095);
-        this.transitions.table.set(TRANSITION_TABLE.table);
+    constructor(
+        terminal?: IParserTerminal | any,


This could be:

constructor( public terminal: IParserTerminal ... )

Tyriar · 2018-04-23T23:08:29Z

src/EscapeSequenceParser.ts

                    if (code > 0x9f) {
                        switch (currentState) {
-                            case 0: // GROUND -> add char to print string
+                            case STATE.GROUND:          // add char to print string


Avoid using whitespace for alignment, align with the : or put on a new line.

Tyriar · 2018-04-23T23:09:58Z

src/EscapeSequenceParser.ts

+                                    code: code,             // actual character code
+                                    state: currentState,    // current state
+                                    print: printed,         // print buffer start index
+                                    dcs: dcs,               // dcs buffer start index


If key and value are the same you can just use one:

{ pos: i, code, state: currentState, print, dcs, ... }

Ah nice shortcut.

Tyriar · 2018-04-23T23:11:46Z

src/EscapeSequenceParser.ts

+
+// FSM actions
+export const enum ACTION {
+    ignore = 0,


Let's use this naming convention for enums:

xterm.js/src/Parser.ts

Lines 153 to 162 in 716a8d5

export enum ParserState {

NORMAL = 0,

ESCAPED = 1,

CSI_PARAM = 2,

CSI = 3,

OSC = 4,

CHARSET = 5,

DCS = 6,

IGNORE = 7

}

Sure thing. Oh and the enums must be const, otherwise Typescript will place JS identifiers into the parse method with worse performance.

Oh wow, that's a huge difference! We must constify all the enums!

enum a { b, c, d } const enum A2 { B2, B3, B4 } console.log(a.b); console.log(A2.B2);

Compiles to:

var a; (function (a) { a[a["b"] = 0] = "b"; a[a["c"] = 1] = "c"; a[a["d"] = 2] = "d"; })(a || (a = {})); console.log(a.b); console.log(0 /* B2 */);

It's probably not enough of an argument against it, but if we ever wanted to use babel to compile the typescript source directly, it's unable to handle const enums, because they require type information.

I am not sure either if you want enums to be const everywhere. I need this feature here to get the plain number at JS level avoiding costly deref at runtime, still I wish I had access to the enum definition (e.g. have the states named for hot patches) and already thought about workarounds (with no handy solution yet lol).

Tyriar · 2018-04-23T23:13:21Z

src/EscapeSequenceParser.ts

-        this.transitions.table.set(TRANSITION_TABLE.table);
+    constructor(
+        terminal?: IParserTerminal | any,
+        transitions: TransitionTable = VT500_TRANSITION_TABLE)


Again you can add public before transitions and then remove the member above.

Tyriar · 2018-04-23T23:13:57Z

src/EscapeSequenceParser.ts

+
+// default transition table points to global object
+// Q: Copy table to allow custom sequences w'o changing global object?
+export class EscapeSequenceParser {


Is this going to hide behind an interface and use the readonly trick for properties that shouldn't be edited outside?

Not sure on this one yet: the transition table takes 5kB as Uint8Array and much more as fallback Array implementation (> 40kB, not done yet). I tend to make the transition table private and give the parser a copy on write method for custom states to save memory for multiple terminals.
Not sure either whether to hide EscapeSequenceParser and give ParserTerminal/InputHandler the main control API.

- quick'n dirty merge of classes - ParserTerminal is subclass of InputHandler - basic fallback handlers implemented - test regressions due to multiple changes in EscapeSequenceParser - runtime down to 2.3s - TODO: fix tests, implement DCS functions, full merge of classes

jerch · 2018-04-29T23:21:28Z

The function lookup tables are in place. The lookup tables are objects without a prototype to cut property lookups. I am not yet happy with the lookup code itself:

ident = collect + String.fromCharCode(code);
if (this._escHandlers[ident]) this._escHandlers[ident](params, collect);
else this._escHandlerFb(collect, code);

There are still some conceptual issues:

Current function map does not allow "half defined" lookups, either a function for a sequence is set or the fallback function will be called (that's a major drawback compared to switch, where you can easily define fall through states).
The function registration does not follow a unique scheme - for CSI a function is registered for flag while DCS and ESC use both collect and flag. The flag method used for CSI is much faster but due to the messed up escape sequences for ESC and DCS where methods for a certain flag have not much in common for different collect values not applicable there.

and minor performance issues:

DCS and ESC keys are strings (10x slower than numercial keys) - How to avoid?
double lookup: if (<lookup>) <lookup>() though I think this gets optimized by the engine and is no biggie

Still it performs better than the switch version (~5% gain), compared to the parser in master this version is ~2.5x faster (320ms vs. 120ms for ls -lR /usr/lib).

I also merged ParserTerminal with InputHandler and moved addChar into the print method which gave some further performance boost (ls -lR /usr/lib runs at 2.3 - 2.5s now). I think in print there is still some room for optimization since the gc kicks in there with ~10% of the total JS runtime (~250 ms). This might be related to the Array creations there, no clue yet.

jerch · 2018-04-30T22:13:19Z

Made some progress towards a nicer integration. The current version does all callback registering directly in InputHandler. I decided to remove ParserTerminal since it mostly passed InputHandler functionality to the parser with almost no own business logic.
Still I dont quite understand the separation level of InputHandler vs. Terminal - some escape sequence actions directly access terminal internals while others do this via an InputHandler method. Maybe the direct accesses could also be managed by InputHandler?

Tyriar · 2018-05-15T15:15:19Z

I can rewrite the OSC API, proside is a more unique "look'n feel" of the API + ~5x speed gain for OSC, downside the rewrite and additional tests. Since it would involve API changes it might be better to do it before this PR lands. What you think?

Let's definitely defer any more changes like this, I want to merge this ASAP to prevent it from becoming stale.

Tyriar · 2018-05-15T15:22:49Z

C0 control codes without an explicit handler - what to do with those? To reassemble the old behavior those should call InputHandler.print. I still think printing those is faulty behavior, but maybe we should do to keep in line with old parser and address it later?

Let's address it later after we're sure there are no major issues with the changes. Shall we create a follow up issue?

C1 control codes - enable or disable? They were not part of xterm.js before, to reassemble old behavior should they be disabled? Btw the old logic would also just print those.

Again let's defer this.

DECRQSS and the valid/invalid flag: I dont quite understand the old code for this but it seems the response always contained 0 as validation indicator. Problem here is the fact that DEC specifies 0 as valid and xterm spec as invalid (they flipped the meaning). I have no clue which one is right but would guess that most modern apps would follow the xterm meaning.

Looks like this was fixed upstream after the fork chjj/term.js@36c1e5c

jerch · 2018-05-15T17:32:49Z

Let's address it later after we're sure there are no major issues with the changes. Shall we create a follow up issue?

Good idea for easier tracking. There is at least one issue that might be affected by this (cant find it atm, was something about a terminal game with weird control code output).

Looks like this was fixed upstream after the fork chjj/term.js@36c1e5c

Yup, it follows the xterm meaning there too. This also contains the DCS sequence for custom keys and a tmux passthrough thing. Might be worth a closer look later on.

Tyriar · 2018-05-21T14:29:24Z

C0 explciit handler: #1461
Enable C1: #1462

Tyriar · 2018-05-21T14:40:30Z

src/InputHandler.ts

+ *   DECRQSS (https://vt100.net/docs/vt510-rm/DECRQSS.html)
+ *   Request Status String (DECRQSS), VT420 and up.
+ *   Response: DECRPSS (https://vt100.net/docs/vt510-rm/DECRPSS.html)
+ *   FIXME: xterm and DEC flip P0 and P1 to indicate valid requests - which one to go with?


Let's fix this before merge

It returns atm 1 for a valid and 0 for an invalid request (xterm style). Should this be flipped? If not I can simply remove the comment.

Ah you're right it is already fixed 😄 let's remove the FIXME

Tyriar · 2018-05-21T15:32:46Z

src/InputHandler.ts

+    switch (this._data) {
+      // valid: DCS 1 $ r Pt ST (xterm)
+      case '"q': // DECSCA
+        return this._terminal.send(C0.ESC + 'P1' + '$r' + '0"q' + C0.ESC + '\\');


These long strings are more readable imo using string interpolation:

${C0.ESC}P1$r0"q${C0.ESC}\\'

Any reason for not going this way?

Will that work with 'ES5' as target? Thought this is ES6 style.

TS compiles it down. This in Renderer.ts:

this._terminal.screenElement.style.width = `${this.dimensions.canvasWidth}px`;

Becomes:

this._terminal.screenElement.style.width = this.dimensions.canvasWidth + "px";

Oh sweet, gonna change it.

…ew_parser

Tyriar · 2018-05-21T17:04:06Z

@jerch I'll merge once I get another go ahead from you 😃

I can pull this into vscode insiders in a few days, I just did a big update with heaps of rendering changes so want to give that some bake time first.

jerch · 2018-05-21T17:06:13Z

Guess I am done with this PR.

Note that I already have some perf optimizations pending for InputHandler.print, I can get them PR'ed as soon as this PR has landed.

Tyriar · 2018-05-21T17:47:58Z

🚀 😨

jerch · 2018-05-21T17:58:41Z

😓

jerch added 4 commits April 21, 2018 05:12

new parser

fb7e5d5

new parser

9b085d2

merge with master

73f40ef

utilize more typescript features

20760d0

jerch mentioned this pull request Apr 22, 2018

proposal: revise usage of property getters #1402

Closed

jerch added 3 commits April 24, 2018 00:21

new parser

f976707

utilize more typescript features

c8910cc

merge with remote

55993ad

Tyriar reviewed Apr 23, 2018

View reviewed changes

jerch added 9 commits April 24, 2018 12:27

move addChar to ParserTerminal

a058395

rename enums

c0ab2b9

first handler API changes

9eb107a

move switches to jump tables

b72cdad

remove term from EscapeSequenceParser

9d2bbd1

Merge branch 'master' into new_parser

27a3347

fix InputHandler

dc14f52

faster print action

0f3d82a

jerch added 9 commits April 30, 2018 01:32

fix import issue

826cca7

some cleanup

a55a7f3

fix error in esc handler call

f5375c3

add array fallback for TransitionTable

a103526

local .buffer in parse

b158176

fix test cases

645cd2e

missing whitespace

dbc6b12

real merge of InputHandler and ParserTerminal

ba6fcdd

cleanup, some docs, temp fix for InputHandler tests

af392a6

Tyriar mentioned this pull request May 11, 2018

Interrupts in integrated terminal microsoft/vscode#49470

Closed

Merge branch 'master' into new_parser

d8afdb1

This was referenced May 17, 2018

window freezes when output large amount of data to Terminal microsoft/vscode#50011

Closed

Implement a canvas renderer #935

Closed

How to correctly handle C0 control codes without an explicit handler #1461

Closed

Enable C1 control codes #1462

Closed

Merge branch 'master' into new_parser

c1f8225

Tyriar requested changes May 21, 2018

View reviewed changes

Tyriar reviewed May 21, 2018

View reviewed changes

jerch added 3 commits May 21, 2018 18:33

remove deprecated FIXME

ceb1432

Merge branch 'new_parser' of https://github.com/jerch/xterm.js into n…

8386ec1

…ew_parser

use string templates in DCS responses

e97882e

Tyriar approved these changes May 21, 2018

View reviewed changes

Merge branch 'master' into new_parser

6bc6ea5

Tyriar merged commit 099c7d8 into xtermjs:master May 21, 2018

This was referenced May 21, 2018

broken escape sequence parser states #145

Closed

proposal: switch to DEC/ANSI compliant escape sequence parser #1395

Closed

Tyriar mentioned this pull request Jun 11, 2018

New terminal parser microsoft/vscode#51615

Closed

Tyriar mentioned this pull request Jul 12, 2018

Box drawing characters with escape sequence Esc ( 0 are broken since 3.5.0 #1565

Closed

Tyriar added the reference A closed issue/pr that is expected to be useful later as a reference label Aug 31, 2018

jerch mentioned this pull request Oct 11, 2018

Improve testing and tracking of performance critical components #1688

Closed

jerch mentioned this pull request Dec 8, 2018

hooks for custom control sequences #1813

Closed

	export enum ParserState {
	NORMAL = 0,
	ESCAPED = 1,
	CSI_PARAM = 2,
	CSI = 3,
	OSC = 4,
	CHARSET = 5,
	DCS = 6,
	IGNORE = 7
	}

new parser #1399

new parser #1399

Conversation

jerch commented Apr 21, 2018

jerch commented Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tyriar Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerch commented Apr 29, 2018

jerch commented Apr 30, 2018

Tyriar commented May 15, 2018

Tyriar commented May 15, 2018

jerch commented May 15, 2018 • edited Loading

Tyriar commented May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerch May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tyriar commented May 21, 2018

jerch commented May 21, 2018

Tyriar commented May 21, 2018

jerch commented May 21, 2018

jerch commented Apr 22, 2018 •

edited

Loading

Tyriar Apr 24, 2018 •

edited

Loading

jerch commented May 15, 2018 •

edited

Loading

Tyriar commented May 21, 2018 •

edited

Loading

jerch May 21, 2018 •

edited

Loading