Copy extended attributes to dest in reflow

[Tyriar]

Fixes #4108

This would throw before:

[jerch]

LGTM.

On a perf sidenote - maybe we should remove the inner for loop above by copying CONTENT, FG and BG the same way explicitly, I think this would run much faster, as loop unrolling in not yet a thing JITs. And it is basically the same with _combined right below. I have not tested that, so this is just a guess maybe worth trying.

[Tyriar]

Do you mean instead of this:

for (let cell = 0; cell < length; cell++) {
        for (let i = 0; i < CELL_SIZE; i++) {

Something like this?

for (let cell = 0; cell < length * CELL_SIZE; cell++) {

I think reflow is relatively fast already considering the amount of work it needs to do already, it might end up bad if you have a lot of content in a bunch of terminals though. I'm also a little scared to touch reflow tbh as I always thought it was a bit fragile.

[jerch]

Something like this?

for (let cell = 0; cell < length * CELL_SIZE; cell++) {

I think reflow is relatively fast already considering the amount of work it needs to do already, it might end up bad if you have a lot of content in a bunch of terminals though. I'm also a little scared to touch reflow tbh as I always thought it was a bit fragile.

Ah nope, messing around with different stepping might be abit too risky imho, I thought more like that (untested, might contain typos as typed directly into the comment field lol):

      for (let cell = 0; cell < length; cell++) {
        const src = (srcCol + cell) * CELL_SIZE;
        const dst = (destCol + cell) * CELL_SIZE;
        this._data[dst + Cell.CONTENT] = srcData[src + Cell.CONTENT];
        this._data[dst + Cell.FG] = srcData[src + Cell.FG];
        this._data[dst + Cell.BG] = srcData[src + Cell.BG];
        if (srcData[src + Cell.CONTENT] & Content.IS_COMBINED_MASK) {
          this._combined[destCol + cell] = src._combined[srcCol + cell];
        }
        if (srcData[src + Cell.BG] & BgFlags.HAS_EXTENDED) {
          this._extendedAttrs[destCol + cell] = src._extendedAttrs[srcCol + cell];
        }
      }
      // + same way for reversed

(Pretty much like manual loop unrolling...)

Edit: Well sorry, I guess I am just side tracking things - imho it is good as it is. Will have a look at the perf idea independently of this.

Edit2: Plz ignore my perf remarks, thats not relevant here, as it is not the hot data intensive code path (mixed that with the resize() method).

Sidenote - I found indeed a perf smell in BufferLine.resize - the new Uint32Array there accounts for ~150ms of 250ms during reflow for a terminal with 100k scrollback x 50 cols (tested with ls -lR /usr filled buffer switching between 50/55 cols). This 150ms could be avoided if we would alloc the buffer en bloc upfront and grab lines slices from it. With such a change reflow would be ~2.5 times faster for big scrollbacks, but the code changes would be quite involved, so meh...

[Tyriar]

@jerch when reducing the size of the buffer, I think we could actually change this:

xterm.js/src/common/buffer/BufferLine.ts

Lines 335 to 336 in f9db65c

	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.

[Tyriar]

#4112