Consider changing null cell data to all zeros

[Tyriar]

On resize when the cols in the terminal increases, we fill in cell data at the end of the buffer here:

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

Line 353 in 1431210

this.setCell(i, fillCellData);

This is doing a relatively large amount of work as each individual index of the cell needs to be set directly off the cell object:

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

Lines 218 to 220 in 1431210

	this._data[index * CELL_SIZE + Cell.CONTENT] = codePoint | (width << Content.WIDTH_SHIFT);
	this._data[index * CELL_SIZE + Cell.FG] = fg;
	this._data[index * CELL_SIZE + Cell.BG] = bg;

If we made "null cell data" all zeroes, we could then use a single typed array fill call, at least when the fill data is all default background and foreground colors which is the typical case.

This would involve either adapting the code so null cells have a width of 0 instead of 1, or change a cell's width=1 to actually be encoded as 0 << Conten.WIDTH_SHIFT:

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

Line 83 in 1431210

this.content = value[CHAR_DATA_CHAR_INDEX].charCodeAt(0) | (value[CHAR_DATA_WIDTH_INDEX] << Content.WIDTH_SHIFT);

[jerch]

Well back then the idea for making the fillCellData explicit was to keep NULL_CELL itself customizable. If thats not needed, we can remove it. Though width encoded as 0 might cause havoc, as it is used in many code places, so thats a tough refactoring.

While thinking about #4115 I found another intriguing refactoring idea, which promises some speedup esp. during print. Currently we maintain a cells in [CONTENT, FG, BG] succession, what if we'd transpose the memory layout to [CONTENT#0, CONTENT#1 ..., FG#0, FG#1 ..., BG#0, BG#1 ...] instead? In print this would shorten the buffer actions to these:

• for char in parser buffer: write CONTENT
• fill(FG, start, start + length of parser buffer)
• fill(BG, start, start + length of parser buffer)

I think any mass action on the buffer working similar like that would benefit from it, as we dont have to do the loadCell/writeCell turnover, but can do copies as 3 mass actions. It has a downside though - during rendering loadCell is still needed as convenient access, but now has to read from non contiguous memory prolly violating cache lines. Whether thats really an issue idk until profiled.

[Tyriar]

@jerch that memory layout would be better for fill, but worse for reusing the content on resize which is currently one of the more problematic areas

[jerch]

Ah right, this would need move semantics on the FG and BG part, which is def. much worse compared to just truncating or filling with zeros.

I also did a quick print shim to test this, it saves ~25% for print runtime (not overwhelming).

About the filling issue above - just checked the bit fields, imho it is safe to just fill NULL_CELLS from its CONTENT even for FG and BG, as the width maps into RGB section, which never gets evaled if CM_MASK is not set in a NULL_CELL (which is currently the case). So we can just do fill(NULL_CELL.CONTENT) for all new excess fields.
Downside: This creates a weird "entanglement" of CONTENT, FG and BG for NULL_CELL and needs some explanation docs to never violate the rule "width in CONTENT always maps into non-evaled regions in FG & BG".

[Tyriar]

@jerch am I reading that right that we can safely male that change, with a warning about width not being accurate if it's CM_MASK isn't set? The default color CM_DEFAULT = 0, we read width when it's the default color?

[jerch]

Ah well I meant it this way - if we just fill all excess uint32 slots from current NULL_CELL.CONTENT value it should just work, because it will just contain (1<<22) in all slots, thus also for FG and BG, where it is technically wrong. But it is not harmful, as the FG/BG value dont eval the lower 3 bytes (RGB section, thus bit 1..24) since CM_MASK is zero. (For CM_MASK==0 the RGB section has no meaning in FG/BG.)
I know thats quite hacky, thus my warning about a proper doc note, if we'd go that route.

In summary we'd only need to do:

• fill excess uint32 slots from current NULL_CELL.CONTENT
• place proper doc warning about "width in NULL_CELL.CONTENT must always map into non-evaled regions in FG & BG"
• and better never touch NULL_CELL repr again (to avoid clashing with that rule)

[Tyriar]

I understand now, it would get the job done but does indeed feel hacky. The optimization probably isn't worth the added complexity 🤔

[jerch]

Yeah - that summarizes it pretty well, sadly. And putting such a hack in place is dangerous, in several months no ones remembers exactly it was all about and boom 🙈