Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parser hooks #2346

Merged
merged 34 commits into from Aug 16, 2019
Merged

Parser hooks #2346

merged 34 commits into from Aug 16, 2019

Conversation

jerch
Copy link
Member

@jerch jerch commented Jul 25, 2019

PR to get the parser actions hookable from outside. Shall fix #2332.

TODO:

  • review OSC handling and hook
    Also introduces OscParser which simplifies EscapeSequenceParser and fixes error in OSC subparsing #2342. OSC handlers now can be attached with full state introspection (START, PUT, END) and UTF32 codepoints. The simplified attaching of a single function as handler is now done with a proxy class OscHandlerFactory. Payload has a hard limit of 10MB.
  • review CSI handling and hook
    Experimental badge removed. Tests were already in place.
  • review ESC handling and create public interface
    Interface and tests added.
  • review DSC handling and create public interface
    Interface added similar to OSC with callback registration (again via a proxy class to catch the substates). Payload has a hard limit of 10MB.

Also closes #1823.

@jerch jerch self-assigned this Jul 25, 2019
@jerch jerch added area/api area/parser type/enhancement Features or improvements to existing features labels Jul 25, 2019
@jerch jerch added this to the 4.0.0 milestone Jul 25, 2019
@jerch jerch mentioned this pull request Jul 25, 2019
11 tasks
src/InputHandler.ts Outdated Show resolved Hide resolved
@jerch
Copy link
Member Author

jerch commented Jul 26, 2019

DCS is halfway done, still missing:

  • dcs parser tests
  • multiple handler tests
  • public interface

@jerch
Copy link
Member Author

jerch commented Jul 26, 2019

@Tyriar Imho there is no formal payload limitation spec'ed for OSC and DCS commands, instead they are meant to work streamlined with chunks (well more precisely on char by char, but thats a perf nightmare in JS thus we chunkify it similar to PRINT). I tried to deal with that by the new OscParser / DcsParser classes, which provide an interface to the chunk loading to IDcsHandler resp. IOscHandler.
At this point we clash with our one shot callback handler registration, that gives the callback the payload as one final big chunk, thus we have to store things in between until the sequence gets finished. To not run into OOM I have hardcoded a payload limit for those handlers of 10 MB (done on the proxy classes OscHandlerFactory and DcsHandlerFactory). Note that most emulators would abort these sequences at much lower limits, maybe @textshell has some numbers here?

@Tyriar You might wonder what changed to before:

Before:

  • OSC had only a "final shot" registration interface and would store all chunks internally on EscapeSequenceParser itself (without payload limit!). This was basically due to my own laziness to expose the "chunk nature" of OSC commands, because all OSC commands that existed 10ys+ ago were rather short. But this changed and other emulators use it to transmit file content (iTerm does file upload and image handling with OSC).
  • DCS had only the chunk/streamline interface before with the complicated IDcsHandler usage/setup (see DECRQSS class in InputHandler.ts). Here the old SIXEL spec clearly indicated that DCS has to be chunkified from the beginning.
  • all lived directly on EscapeSequenceParser, which was rather cluttered

Now:

  • both have the chunk/streamline interface internally
  • both have now a "final shot" registration interface with payload limits
  • better separation of concerns in EscapeSequenceParser, OscParser and DcsParser
  • public API only exposes the "final shot" interfaces, which is imho enough for most use cases (note that the chunk interface does not have the payload limitation)

Edit: Lowered the limit to 10 MB, with 50 MB nodejs segfaults (prolly due to the string conversion).

@jerch
Copy link
Member Author

jerch commented Jul 26, 2019

Up for review.

typings/xterm.d.ts Outdated Show resolved Hide resolved
@jerch
Copy link
Member Author

jerch commented Jul 31, 2019

Done with the rework of function registration. To see the impact here are some benchmark numbers:

"PRINT - a"
  master: 619.79 MB/s
  PR    : 626.24 MB/s
"EXECUTE - \n"
  master: 98.41 MB/s
  PR    : 110.16 MB/s
"ESCAPE - ESC E"
  master: 57.01 MB/s
  PR    : 49.53 MB/s
"ESCAPE with collect - ESC % G"
  master: 22.33 MB/s
  PR    : 62.60 MB/s
"CSI - CSI A"
  master: 92.12 MB/s
  PR    : 86.78 MB/s
"CSI with collect - CSI ? p"
  master: 58.06 MB/s
  PR    : 74.98 MB/s
"CSI with params (short) - CSI 1;2 m"
  master: 101.13 MB/s
  PR    : 97.11 MB/s
"CSI with params (long) - CSI 1;2;3;4;5;6;7;8;9;0 m"
  master: 146.17 MB/s
  PR    : 141.96 MB/s
"OSC (short) - OSC 0;hi ST"
  master: 52.22 MB/s
  PR    : 55.69 MB/s
"OSC (long) - OSC 0;<text> ST"
  master: 48.43 MB/s
  PR    : 65.18 MB/s
"DCS (short)"
  master: 61.88 MB/s
  PR    : 45.79 MB/s
"DCS (long)"
  master: 253.15 MB/s
  PR    : 214.90 MB/s

Across all functions its pretty equal. Still interesting that removing the intermediate string concat for "ESCAPE with collect - ESC % G" shows almost 3x boost while "CSI with collect - CSI ? p" is only slightly faster. OSC seems to benefit slightly from the OscParser while DCS perf decreased (prolly due looping into registered handlers, before it was a single handler).

@jerch
Copy link
Member Author

jerch commented Jul 31, 2019

Introduced a new interface to the API for the callback registration:

  export interface IFunctionIdentifier {
    prefix?: string;
    intermediates?: string;
    final: string;
  }

This data type is used during callback registration of CSI, DCS and ESC sequences on the parser to create numerical ids as the jump table keys. As shown above this greatly speeds up sequences with intermediates.

Dispatching rules:

  • other than before handlers register now on one IFunctionIdentifier endpoint, thus if a handler should be reused for a slightly different endpoint, it has to be registered explicitly for that too, example:
    this._parser.setCsiHandler({final: 'J'}, params => this.eraseInDisplay(params));
    this._parser.setCsiHandler({prefix: '?', final: 'J'}, params => this.eraseInDisplay(params));
  • if no handler was attached a fallback handler will be triggered as before (we currently install those with debug notifications like "unknown sequence XY ...", inspectable with {logLevel: debug} in the demo)

Again up for review.

@Tyriar Tyriar added the breaking-change Breaks API and requires a major version bump label Aug 2, 2019
@jerch jerch mentioned this pull request Aug 4, 2019
3 tasks
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any parts in particular that need me to review? Some of the internals of EscapeSequenceParser are a bit tough for me to interpret these days, I like that OscParser/DcsParser logic is moving out of that file though.

typings/xterm.d.ts Outdated Show resolved Hide resolved
src/common/parser/OscParser.ts Outdated Show resolved Hide resolved
@jerch
Copy link
Member Author

jerch commented Aug 7, 2019

Any parts in particular that need me to review?

@Tyriar Well the payload limit of 10MB bugs me abit, but I think this can be increased with a string concat patch for chrome once we have a use case for a DCS/OSC command using that much payload.
Also could you have a look on the sub parsers dispose code? Just in case I overlooked something.

src/public/Terminal.ts Outdated Show resolved Hide resolved
src/InputHandler.ts Show resolved Hide resolved
src/InputHandler.ts Outdated Show resolved Hide resolved
src/Types.d.ts Outdated Show resolved Hide resolved
src/common/parser/Constants.ts Show resolved Hide resolved
src/common/parser/DcsParser.ts Outdated Show resolved Hide resolved
src/common/parser/OscParser.ts Show resolved Hide resolved
src/common/parser/Types.d.ts Outdated Show resolved Hide resolved
src/public/Terminal.ts Outdated Show resolved Hide resolved
typings/xterm.d.ts Show resolved Hide resolved
src/common/parser/EscapeSequenceParser.ts Outdated Show resolved Hide resolved
typings/xterm.d.ts Show resolved Hide resolved
@jerch jerch merged commit 5a9e6fe into xtermjs:master Aug 16, 2019
@jerch jerch deleted the parser_hooks branch August 16, 2019 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api area/parser breaking-change Breaks API and requires a major version bump type/enhancement Features or improvements to existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

error in OSC subparsing Rework/sanitize parser hooks cleanup/enhancement of parser
3 participants