Parser refactor and optimization by Tyriar · Pull Request #462 · xtermjs/xterm.js

Tyriar · 2017-01-09T00:23:21Z

Fixes #459

This PR refactors the parser to improve performance and maintainability. Here are the specifics:

Pull parsing logic from xterm.js into Parser.ts
Use a set of maps when parsing so that the operation is O(1) not O(n), where n is the number of possibilities, this is the main performance gain
Pull all input/code handling into InputHandler
Pull charsets into Charsets.ts
Remove empty functions and commented out code for unimplemented codes, this old code will always be available by looking into git history or at chjj/term.js

By looking at the top of Parser.ts you can now see much more easily what is implemented and what is not, eg. p is implemented for prefix ! only:

csiStateHandler['p'] = (handler, params, prefix) => {
  switch (prefix) {
    case '!': handler.softReset(params); break;
  }
};

Parsing performance of regular characters is expected to improve ~20% (much more when parsing CSI codes). These are things I have in mind for what to follow this up with:

Implement more codes much more easily (eg. Support set cursor style (DECSCUSR) #480)
Implement other charsets Support other character sets #481
Add parser tests much more easily due to IInputHandler and separation of Terminal and Parser (also fix the escape-sequences-test.js tests we're not passing)
Link Terminal and InputHandler closer, perhaps using an InputHandlingTerminal which extends Terminal and implements IInputHandler?
Improve InputHandleraddChar, pull wcwidth into own module, etc.
Move towards removing Parser's dependence on Terminal so that the Terminal requests parsing from Parser which passes everything along to InputHandler
Move parser variables (params, currentParam, prefix postfix, etc) into Parser`
Move more out of xterm.js (eventually creating Terminal.ts and xterm.ts (entrypoint) which exposes it

Part of xtermjs#459

Tyriar · 2017-01-11T22:50:03Z

I wrote a little benchmark and this PR is already showing great improvements! The regression below is probably due to overhead in funneling the calls through InputHandler -> Terminal which is a temporary situation.

Before:

  Parser
500000 iterations time: 778ms
Average iteration time : 0.001556ms
    ✓ Single character parse performance (779ms)
500000 iterations time: 18131ms
Average iteration time : 0.036262ms
    ✓ Simple character long line parse performance (18132ms)

After:

  Parser
500000 iterations time: 891ms
Average iteration time : 0.001782ms
    ✓ Single character parse performance (891ms)
500000 iterations time: 15435ms
Average iteration time : 0.03087ms
    ✓ Simple character long line parse performance (15435ms)

On a related note, phantomjs unit tests are fairly easy via mocha-phantomjs.

parisk · 2017-01-13T08:45:13Z

Great, I think that the one-char overhead won't be even noticed, so we don't have to worry about it 👍 .

Tyriar · 2017-01-13T15:08:09Z

The overhead also appears to have gone away for the most part which long lines with no escape sequences are ~20% faster.

parisk · 2017-01-16T22:45:22Z

This PR seems to have taken a great course 👍 .

The whole process of input handling is much cleaner right now.

@Tyriar do you have any to-dos in your mind that have to be completed before moving on with reviewing an merging this? If yes, can you put them in the description?

Tyriar · 2017-01-16T22:57:31Z

@parisk I don't think so, there are some TODOs in the code but they are questions and/or can be deferred. I'll update the main PR comment to reflect current state (ready for review) and what is included.

I just did an implementation of CSI Ps SP q #480 which demonstrates what makes up a CSI code implementation now which will probably help you understand the flow Tyriar@1a67893

Tyriar · 2017-01-16T23:14:34Z

@parisk updated

parisk

Just some documentation needed.

parisk · 2017-01-18T16:53:10Z

src/Charsets.ts

@@ -0,0 +1,58 @@
+// TODO: Give this a proper type


Add license declaration based on #388?

parisk · 2017-01-18T16:53:22Z

src/InputHandler.ts

@@ -0,0 +1,1522 @@
+import { IInputHandler, ITerminal } from './Interfaces';


Add license declaration based on #388?

parisk · 2017-01-18T16:53:57Z

src/InputHandler.ts

+import { C0 } from './EscapeSequences';
+import { CHARSETS } from './Charsets';
+
+export class InputHandler implements IInputHandler {


Can you add JSDoc comment please?

parisk · 2017-01-18T16:54:01Z

src/InputHandler.ts

+  // TODO: We want to type _terminal when it's pulled into TS
+  constructor(private _terminal: any) { }
+
+  public addChar(char: string, code: number): void {


Can you add JSDoc comment please?

parisk · 2017-01-18T16:54:40Z

src/InputHandler.ts

+   * BEL
+   * Bell (Ctrl-G).
+   */
+  public bell(): void {


Can you explain in the JSDoc comment what this method is about (ringing the "bell" of the terminal).

parisk · 2017-01-18T17:03:40Z

src/Parser.ts

+    this._terminal.currentParam = param;
+  }
+
+  public getParam(): number {


Can you explain a little bit more in the JSDoc comment what this method does and what does it return?

parisk · 2017-01-18T17:03:47Z

src/Parser.ts

+    return this._terminal.currentParam;
+  }
+
+  public finalizeParam(): void {


Can you explain a little bit more in the JSDoc comment what this method does?

parisk · 2017-01-18T17:04:00Z

src/Parser.ts

+    this._terminal.currentParam = 0;
+  }
+
+  public setPostfix(postfix: string): void {


Can you explain a little bit more in the JSDoc comment what this method does and document arguments as well?

parisk · 2017-01-18T17:04:08Z

src/Parser.ts

+    this._terminal.postfix = postfix;
+  }
+
+  public skipNextChar(): void {


Can you explain a little bit more in the JSDoc comment what this method does?

parisk · 2017-01-18T17:04:20Z

src/Parser.ts

+    this._position++;
+  }
+
+  // public repeatChar(): void {


Should we delete these comments?

I kept this in as it will be needed when we convert more to use maps, comments are removed not from output so it doesn't hurt us at all.

parisk · 2017-01-18T17:05:45Z

Great job @Tyriar! The code is much more readable right now and maintainable. Tests run successfully and everything seems to be working great.

The only thing missing is some documentation comments and I think we can move on with merging 🚀 .

Tyriar · 2017-01-18T17:36:46Z

@parisk I added jsdoc, I didn't document the individual input handler methods but instead in InputHandler's class comment referred to http://invisible-island.net/xterm/ctlseqs/ctlseqs.html which gives more context.

Tyriar · 2017-01-18T18:01:58Z

Here goes 😅

Tyriar added 2 commits January 8, 2017 16:11

Create InputHandler which performs C0 actions

04b1ebf

Part of xtermjs#459

Handle C0 codes via handler map in Parser

659fb90

Tyriar added the work-in-progress Do not merge label Jan 9, 2017

Tyriar self-assigned this Jan 9, 2017

Tyriar requested a review from parisk January 9, 2017 00:23

Tyriar added 9 commits January 8, 2017 17:12

Add C0.BS handler back

665a11a

Move parsing logic into Parser.ts

a31921a

Move wcwidth into Parser.ts

4a27b9a

Fix wcwidth TS errors

940bd26

Fix references to Terminaal and fix TS lint warnings

991b284

Pull charsets out of xterm.js

1848fff

Merge remote-tracking branch 'upstream/master' into 459_parser__on_460

f4267c4

Move all parser state handling into the parser

6deaaa8

Start pulling CSI handlers out

672dc1a

Tyriar force-pushed the 459_parser__on_460 branch from f7710a4 to 672dc1a Compare January 9, 2017 23:17

Tyriar added 3 commits January 9, 2017 15:19

Handle CSI postfix

635a4d7

Start converting CSI codes

9942477

Remove IInputHandler from CSI_PARAM lambdas

7572dd5

Tyriar force-pushed the 459_parser__on_460 branch from 49ece2d to 7572dd5 Compare January 9, 2017 23:40

Tyriar added 3 commits January 9, 2017 16:07

Handle remaining CSI_PARAM cases and introduce the state

c7fa2d5

Support more CSI codes, remove unused CSI helper functions

db81c28

Add CSI @, E, F, G

411b80c

This was referenced Jan 10, 2017

Pull C0 escape sequences into its own file #461

Merged

Change to xterm.js vercel/hyper#1275

Closed

Improve xterm.js performance microsoft/vscode#17875

Closed

Tyriar changed the title ~~Parser refactor~~ Parser refactor and optimization Jan 11, 2017

Merge remote-tracking branch 'upstream/master' into 459_parser__on_460

342e862

Tyriar added 2 commits January 12, 2017 20:32

Convert more CSI codes

f9a286a

Add more CSI codes

c43c3b4

Tyriar added 4 commits January 12, 2017 21:33

Add more CSI codes

9b66208

Finish CSI codes

f4846aa

Pull character add char code to input handler

fa3484c

Start adding ESC state handler

ecd7e69

Tyriar mentioned this pull request Jan 15, 2017

Support set cursor style (DECSCUSR) #480

Closed

Use charsets to map

4448ebe

Tyriar mentioned this pull request Jan 15, 2017

Support other character sets #481

Closed

Tyriar added 2 commits January 16, 2017 13:53

Merge remote-tracking branch 'upstream/master' into 459_parser__on_460

5e1ccb3

Remove parser states from xterm.js

1cdd2e0

Tyriar removed the work-in-progress Do not merge label Jan 16, 2017

Merge remote-tracking branch 'upstream/master' into 459_parser__on_460

e2b2f98

parisk suggested changes Jan 18, 2017

View reviewed changes

Add documentation

ef60e50

parisk approved these changes Jan 18, 2017

View reviewed changes

Tyriar merged commit 5bd8033 into xtermjs:master Jan 18, 2017

Tyriar deleted the 459_parser__on_460 branch January 18, 2017 18:49

Tyriar modified the milestone: 2.3.0 Feb 2, 2017

		@@ -0,0 +1,1522 @@
		import { IInputHandler, ITerminal } from './Interfaces';

Conversation

Tyriar commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tyriar commented Jan 11, 2017

Uh oh!

parisk commented Jan 13, 2017

Uh oh!

Tyriar commented Jan 13, 2017

Uh oh!

parisk commented Jan 16, 2017

Uh oh!

Tyriar commented Jan 16, 2017

Uh oh!

Tyriar commented Jan 16, 2017

Uh oh!

parisk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parisk commented Jan 18, 2017

Uh oh!

Tyriar commented Jan 18, 2017

Uh oh!

Tyriar commented Jan 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Tyriar commented Jan 9, 2017 •

edited

Loading