xterm.js icon indicating copy to clipboard operation
xterm.js copied to clipboard

Avoid creating new ArrayBuffers when reducing the size of the buffer

Open Tyriar opened this issue 2 years ago • 6 comments

See https://github.com/xtermjs/xterm.js/pull/4109#issuecomment-1243690016

I think we could actually change this:

https://github.com/xtermjs/xterm.js/blob/f9db65c52e4702cf906c14cda1c640342a7b6dca/src/common/buffer/BufferLine.ts#L335-L336

To:

this._data = this._data.subarray(0, cols * CELL_SIZE);

That will keep the same ArrayBuffer, but use a different view of it. We wouldn't free the memory but maybe we should only do that either when the view is < 1/2 the size of the buffer, or defer the clean up to an idle timeout? It doesn't make sense to do so many allocations if they're being thrown away immediately after several resizes.

We could do something similar here:

https://github.com/xtermjs/xterm.js/blob/f9db65c52e4702cf906c14cda1c640342a7b6dca/src/common/buffer/BufferLine.ts#L347-L348

Tyriar avatar Sep 12 '22 12:09 Tyriar

@Tyriar This a really good idea, it would lower CPU stress for multiple shrinking events in a row. The halving also sounds good to me, it would also lift some burden from expanding resizes within the old upper limit.

Are the resize events de-bounced? Like the event would not get triggered for every single number, if one changes from 80 to 50 cols fast enough?

jerch avatar Sep 13 '22 08:09 jerch

Did a quick test with a 100k scrollback terminal filled (only testing col shrinking): old ~320ms vs. new ~160ms for resize runtime - speed doubled. :smiley_cat:

Seems nodejs does not support requestIdleCallback due to the way the libuv event loop works. So this careful shimming for the headless variant to not let it hog tons of memory.

jerch avatar Sep 13 '22 08:09 jerch

Here a few numbers with the new approach. Again done on 100k scrollback, from shriniking/enlarging between 55 -65 cols. They should be JIT'ed (did some warmup before profiling), and show 2 +-1 col events:

  • shrink by one col: image

  • enlarge by one col w'o new buffer alloc/copy (within buffer.bytelength): image

  • enlarge by one col with full buffer alloc/copy (outside of buffer.bytelength): image

Without the patches all actions look like the last image. With the patches we gain:

  • shrinking ~4x faster
  • enlarging within bytelength ~1.9x faster
  • enlarging outside bytelength - same speed as before

Further take aways:

  • main cost during resize comes from alloc + data moving - alloc accounts for ~half of the runtime, avoiding all data moves gives another 2x speedup (as seen for shrinking where no data has to be moved in BufferLine.resize)
  • getTrimmedLength has constant runtime of ~25ms (~10%), prolly not worth be optimized further
  • _reflowSmaller has constant runtime of ~80 ms, accounting for ~45% during shrinking and 12 - 30% during enlarging

@Tyriar Idk how your reflow logic works, but wonder why _reflowSmaller would appear for enlarging at all?

jerch avatar Sep 13 '22 10:09 jerch

Something weird is going on during enlarging in the demo. If I increase cols by one I get the following onResize dispatching:

image

Note that I changed cols to 87, which is correctly announce by the first onResize event, but gets reverted by a second event shrinking to 86. Therefore cols in the demo is off by one (stty size also reports 86 afterwards). For some reason the resize dispatching is messed up. It also explains why I see _reflowSmaller above in the profiler stats.

Edit: Oh well, this is a result of updateTerminalSize() in client.ts and FitAddon.fit not yielding the same numbers, if you have a zoom level on the browser (which I had by accident). With zoomlevel 100% I dont get the re-adjustment to cols-1 anymore. So the numbers above are flawed for enlarging (subtract the _reflowSmaller runtime). Not sure if this is worthing being fixed in client.ts, it might create frictions on integrator side, if they c&p the code? (created #4113).

jerch avatar Sep 13 '22 10:09 jerch

Runtimes w'o zoomlevel madness:

  • shrinking ~85ms (~3x faster)
  • enlarging within bounds ~93ms (~2.7x faster)
  • enlarging out of bounds ~250ms (same speed)

jerch avatar Sep 13 '22 11:09 jerch

Are the resize events de-bounced?

This is up to the embedder. It's debounced by 50ms in VS Code as we still want it to be relatively responsive, so multiple resizes can easily happen in a second.

Tyriar avatar Sep 13 '22 13:09 Tyriar