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

Avoid creating new ArrayBuffers when reducing the size of the buffer #4112

Closed
Tyriar opened this issue Sep 12, 2022 · 6 comments · Fixed by #4115
Closed

Avoid creating new ArrayBuffers when reducing the size of the buffer #4112

Tyriar opened this issue Sep 12, 2022 · 6 comments · Fixed by #4115

Comments

@Tyriar
Copy link
Member

Tyriar commented Sep 12, 2022

See #4109 (comment)

I think we could actually change this:

const data = new Uint32Array(cols * CELL_SIZE);
data.set(this._data.subarray(0, cols * CELL_SIZE));

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:

this._data = new Uint32Array(0);
this._combined = {};

@jerch
Copy link
Member

jerch commented Sep 13, 2022

@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
Copy link
Member

jerch commented Sep 13, 2022

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

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
Copy link
Member

jerch commented Sep 13, 2022

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
Copy link
Member

jerch commented Sep 13, 2022

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
Copy link
Member

jerch commented Sep 13, 2022

Runtimes w'o zoomlevel madness:

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

@Tyriar
Copy link
Member Author

Tyriar commented Sep 13, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants