Reflow text on resize

[Tyriar]

So I started working on the solution for reflow I designed in #622 (comment) and ran into some problems with dealing with how the escape sequences impact the buffer. It got me thinking, maybe reflow will be fast enough by just adjusting the whole buffer with the new buffer implementation. Instead of dealing with copying a bunch of strings like before or recording wrapping points, now we just need to copy data to and from a bunch of ArrayBuffers and inserting or deleting extra BufferLines.

I'm going to defer benchmarking until the todo list below is closer to done, but the results look good so far, along the lines of ~5-20ms for 1000 scrollback, definitely seems good enough to go ahead with. Also, if perf is an issue on less powerful machines or higher scrollbacks, there is always the option of debouncing resize.

Note that this only works when the TypedArray buffer is enabled.

Fixes #622 (finally)
Part of microsoft/vscode#23688

---

TODO

• Fix tests
• Add tests
• Disallow BufferLines to store more cells than Terminal.cols
• Verify with @jerch I removed the right buffer tests relating to shrinking
• Make sure it didn't break with JsArray (or just remove that impl outright)
• Only enable reflow on the normal buffer
• Make sure ydisp/ybase cases are right
• Use set where appropriate
• Handle combining chars
• Optimize the critical sections
  • Make reflow smaller faster
  • Make reflow larger faster
• Fix markers when lines are inserted/removed
• Do some benchmarking

Possible improvements in the future

• Don't resize down BufferLine._data unless it saves 50%+ memory, this would save a bunch of runtime in BufferLine.resize in creating/gcing new/old arrays Don't resize down BufferLine._data unless it saves a significant amount of memory #1870
• Only reflow up to the top of the viewport, need to consider implications of this on other components like search. Maybe reflow up to whatever line translateToString is called on? Only reflow to the top of the viewport #1869

---

Automated test on demo

Exposing client.ts's updateTerminalSize on window and running this will alternate resizing between 10 and 100:

var direction = -1;
setInterval(() => {
    if (direction < 0 && term.cols === 10 || direction > 0 && term.cols === 100) {
        direction *= -1;
    }

    <!-- term.resize(term.cols + direction, term.rows); -->
    document.querySelector('#opt-cols').value = term.cols + direction;
    window.updateTerminalSize();
}, 40);

[jerch]

Wow nice, this looks very promising 👍

[jerch]

Few more remarks from my side:

• markers: How we gonna deal with them? Imho the more advanced proposal of decorators acting on col/row ranges (Allow embedders to decorate ranges in the buffer #1852) could be used here to deal with the oldCol/oldRow --> newCol/newRow transition.
• speed: Did some first benchmarks - it shows ~25 ms for 1000 lines scrollback in the demo (scales linear, ~250 ms for 10k lines). Surprisingly most of the time is spent in CircularList.splice, any idea why? Maybe this can be further optimized once we fixed the logic. I was expecting to see much more time spent in content copying.
• debounce: Yeah imho we should debounce the resize event, as it will be fired several times in a row for mouse based window resize.

[Tyriar]

markers: How we gonna deal with them? Imho the more advanced proposal of decorators acting on col/row ranges (#1852) could be used here to deal with the oldCol/oldRow --> newCol/newRow transition.

Well at the very least we could iterate over all the markers and adjust them as appropriately.

speed: Did some first benchmarks - it shows ~25 ms for 1000 lines scrollback in the demo (scales linear, ~250 ms for 10k lines). Surprisingly most of the time is spent in CircularList.splice, any idea why? Maybe this can be further optimized once we fixed the logic. I was expecting to see much more time spent in content copying.

CircularList.splice is very expensive as it needs to shift all the elements when inserting or deleting:

xterm.js/src/common/CircularList.ts

Lines 141 to 144 in 4375b3c

	for (let i = start; i < this._length - deleteCount; i++) {
	this._array[this._getCyclicIndex(i)] = this._array[this._getCyclicIndex(i + deleteCount)];
	}
	this._length -= deleteCount;

One optimization that comes to mind is to more manually control CircularList from Buffer so that we're only doing this shifting once.

[jerch]

One optimization that comes to mind is to more manually control CircularList from Buffer so that we're only doing this shifting once.

Would be great if we get it down to just one "cleanup step". Id expect it to be magnitudes faster and less growing with the amount of lines 😸

[Tyriar]

Just sped up reflow smaller greatly. I tried 2 solutions to optimize:

1. Split the work into 2 phases; the first gathers which rows are going where, the second does the reflow and assigns to the correct row. This ended up super complex since rows can shift both up and down based on whether scrollback is full or not.
2. Keep track of lines to insert during reflow in an array, then after reflow go through the lines and correct all the positions.

[jerch]

I think we should fix the wide char thing before merging as it might affect a pretty big user base.

[jerch]

The emoji is actually a surrogate pair (which happens to be handled like a combined string currently, but not with upcoming the UTF32 buffer anymore). Would be better to test combining here with an accent like '`'.

We also need some wide char tests as they have that awkward "dangling cell" behavior (Have not seen tests with wide chars)

[Tyriar]

Would be better to test combining here with an accent like '`'.

Not sure what you mean here?

We also need some wide char tests as they have that awkward "dangling cell" behavior (Have not seen tests with wide chars)

I think these are covered by the wide tests now?

[jerch]

@Tyriar Great job! My chinese lorem ipsum test is now working correctly, it does not add white spaces anymore.

On minor thing I found: The cursor gets not moved, if its on a wrapped line. Not even sure what the right way to deal with it, whether it should move with the reflowing content or stay at its position. I think we can address this as soon as issues come in.

Ready from my side. 👍