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

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

Closed
jerch opened this issue Apr 19, 2018 · 4 comments
Closed

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

jerch opened this issue Apr 19, 2018 · 4 comments
Labels
type/enhancement Features or improvements to existing features
Milestone

Comments

@jerch
Copy link
Member

jerch commented Apr 19, 2018

Since the current main parser does not cover some well defined exceptional escape code conditions I want to propose to switch over to this reference parser https://vt100.net/emu/dec_ansi_parser, which covers 99% of the DEC terminal sequences up to the vt500 series.

I've done a JS implementation years ago that could be adopted to xterm.js (https://github.com/netzkolchose/node-ansiparser). It is based on a FSM approach, adoption should be straight forward with a parser-terminal interface. The FSM could be extended to support pluggable custom escape codes. Speed should be no concern (runs at 80MB/s average, with minor tweaks at 100MB/s).

Switching over to that parser would raise the DEC/ANSI compatibility to almost 100%.

@Tyriar
Copy link
Member

Tyriar commented Apr 19, 2018

@jerch I'd prefer to take more incremental (less radical) steps to improve the parser. Switching it out completely would surely introduce a bunch of regressions that would cause issues for me. Putting tests in place and transitioning the parser over to something closer to this would be a better plan. I'd also prefer if everything lived inside the xtermjs org (and this repo unless there's a need to share).

I really like the idea of pulling out a generic parser and then having a class on top of that which tells it how to act (gives it TRANSITION_TABLE). Improving throughput of the parser is one of the bigger perf bottlenecks currently, as well as having it optionally live on a web worker so it doesn't block the renderer.

@jerch === @netzkolchose?

@Tyriar Tyriar added the type/enhancement Features or improvements to existing features label Apr 19, 2018
@jerch
Copy link
Member Author

jerch commented Apr 19, 2018

I'd prefer to take more incremental (less radical) steps to improve the parser. Switching it out completely would surely introduce a bunch of regressions that would cause issues for me.

Yeah well the interface glue code would need to be tested. Not sure if the current terminal implementation makes some assumptions about faulty escape sequence handling, if so this would have to be stripped as well.

I'd also prefer if everything lived inside the xtermjs org (and this repo unless there's a need to share).

Thats no problem for me (I am the only contributor), the code could easily live in this repo as some typescript thingy.

Improving throughput of the parser is one of the bigger perf bottlenecks currently, as well as having it optionally live on a web worker so it doesn't block the renderer.

Not sure how the current parser performs at the moment, the 100MB/s was the fastest I could get node-ansiparser (was initially translated from a c project and is at ~50% of the c parser). With a strict separation of concerns at low level moving parts to a web worker should be much easier (saw that the current parser also does some of the terminal state handling while node-ansiparser is more a tokenizer and just invokes the needed callbacks).

@jerch === @netzkolchose?

Lol nope, the others are just lazy to opensource their stuff 😉

@Tyriar
Copy link
Member

Tyriar commented Apr 20, 2018

saw that the current parser also does some of the terminal state handling while node-ansiparser is more a tokenizer and just invokes the needed callbacks

Splitting this out would definitely be a good move 😄

@jerch
Copy link
Member Author

jerch commented May 22, 2018

Fixed with #1399.

@jerch jerch closed this as completed May 22, 2018
@Tyriar Tyriar added this to the 3.5.0 milestone May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Features or improvements to existing features
Projects
None yet
Development

No branches or pull requests

2 participants