performance optimizations

[jerch]

PR to gather some low hanging fruit performance optimizations.

As benchmark I use the command ls -lh /usr/lib on ubuntu and track the second run to avoid the altas rendering stuff.

JS timings:

1. master: 4.8s
2. make .buffer local where appropriate: 3.2s
3. 2nd change + direct access to Buffer.lines: 2.9s
4. 2nd + prefetch row in addChar: 2.9s

[mofux]

Nice! Do you think it would be beneficial if we'd do the same for the buffer.lines accessor?
https://github.com/xtermjs/xterm.js/blob/master/src/Buffer.ts#L52

[jerch]

@mofux Not sure yet, I'll give it a try. Also I would try to omit API changes as much as possible, lets see how far we can get with that 😀

[Tyriar]

Looks good! I think all the new buffer local can be declared const is the only comment

[jerch]

@Tyriar Sure thing.

The second change is abit smelly, it maps Buffer.lines directly to Buffer._lines with the drawback, that everytime _lines get initializes it must be set to lines as well. Not so nice and might lead to errors because one might forget the second step. I am not even sure I caught all _lines = new ... invocations.

[mofux]

@jerch Thanks! Did you see any noticeable performance improvements by getting rid of the lines getter? If so it might be wise to get rid of the _lines variable and point any _lines references to lines? I'm actually not sure why we decided to make lines a getter in the first place 🤔

[jerch]

@mofux Well yes it is ~5 - 10% boost, varies from run to run.

[jerch]

Hmm weird thing I just observed in chrome - performance is degrading with some time:

My test loop is "reload page", "run command once", "reset performance stats", "record", "run command again", "get timings from last run" - and that several times. I run it by hand (no selenium whatsoever), and with several runs the times get worse. After relaunching chrome it is back to normal. The degrading seems to keep going, therefore I think it is not only GC stuff related from the dev console.

Do we have any long running perf data? Is this known to happen for long running sessions? A chrome bug maybe? Note that I reload the page in between.

[mofux]

@jerch I remember seeing a similar effect a while ago with another project. I think the Chrome dev tools cause this because they remain connected to the process while the page reloads. You might try to close the dev-tools before you reload. You could also try to run each test in a fresh new tab.

[jerch]

@mofux Ah yepp changing the tab worked, thanks.

The hot sh*t happens in addChar for tons of input, the method itself eats 30% of the JS runtime. Maybe this method can be further optimized.

Therefore the last commit prefetches the row, which kinda makes the Buffer._lines thing obsolete, at least performance wise it has no impact anymore.

[jerch]

Guess I am done, cannot squeeze more out of it.

Summary

1. make .buffer local gives huge boost of 30 - 40 % (stable around 3.3s)
2. changing Buffer.lines gives small boost of 5 - 10 % (relative stable around 3s with 1.)
3. prefetching row in addChar gives small boost of 5 - 10 % (relative stable around 3s with 1.)

1 & 2, 1 & 3 are stacking, 2 & 3 are not stacking since they kinda address the same problem.

Imho 1. is worth to be included, although local method code gets a little longer with all those prefetching. I am unsure about 2 and 3, if I had to vote for one I'd say 3 since it is only a local change.

[Tyriar]

changing Buffer.lines gives small boost of 5 - 10 % (relative stable around 3s with 1.)

Is everything using IBuffer and not Buffer? Let's just merge Buffer._lines into Buffer.lines and change the interface to this:

export interface IBuffer {
  readonly lines: ICircularList<LineData>;
}

That will achieve the same effect without a getter.

[jerch]

Final PR up for review - contains now the local .buffer and the removal of the _lines as suggested by Tyriar. Timing is stable at 3s, which means 60% Javascript perfomance boost.

@Tyriar : The readonly flag did not work due to an assignment to it in Linkifier.test.ts. - Fixed with a workaround for MockBuffer.

[Tyriar]

@jerch good workaround, it's fine to cast or do more dodgy stuff in tests 👍