Stabilize buffer API

[Tyriar]

Introduced in #2074, we should remove experimental after we know confirm no changes are needed for the search addon to function.

[jerch]

@Tyriar Made some progress in that field in #2369 together with @JavaCS3, see this comment #2369 (comment) and my changes here: master...jerch:serialize_with_private#diff-3cf93a85b230e197db3cdf45b888018f

I am sure you not gonna like the changes much, basically it seems we should still avoid getters.

[Tyriar]

@jerch this blocks the v4 release as we should stabilize such that we don't run into issues with the search addon having broken API in a later v4 release. How about instead converting all the getters into a function we preemptively fetch all the line's values and expose them as regular properties. It's readonly on the API and doesn't matter much if people try to workaround it anyway.

So instead of this:

return new BufferCellApiView(this._line, x);

...

class BufferCellApiView implements IBufferCellApi {
  constructor(private _line: IBufferLine, private _x: number) {}
  public get char(): string { return this._line.getString(this._x); }
  public get width(): number { return this._line.getWidth(this._x); }
}

We do this instead:

return {
    char: this._line.getString(x),
    width: this._line.getWidth(x)
}

Or this:

return new BufferCellApiView(this._line, x);

...


class BufferCellApiView implements IBufferCellApi {
  public char: string;
  public width: number;
  constructor(line: IBufferLine, x: number) {
    this.char = line.getString(x);
    this.width = line.getWidth(x);
  }
}

Reasons I don't think we should do all functions:

• We would be breaking existing (experimental) API, that would result in the search addon for example breaking for all future versions. While we can do this, it is a bit of a pain, plus VS Code uses the buffer API.
• Functions are uglier to use than properties, it would be best if we would be consistent across the API ( IBufferApi.cursorY, IMarker.line, Terminal.element, etc.).
• While there will always be faster ways, they aren't as ergonomic and there are ways around it (don't serialize as much, support incremental serialization, etc.).

/cc @JavaCS3

[jerch]

@Tyriar

You want to accept an API slowdown of 4 - 10 times for the sake of writing cell.xy instead of cell.getXY(), seriously? This is not about something being slightly worse in runtime, this is magnitudes slower for use cases that potentially run quite often and will create hiccups (as any outer custom functionality based on buffer data will have to go through that layer).

A major version jump is the only right place to address things like that, if we miss it now its not for a better xterm.js because ppl will jump on that API and we prolly cannot undo it later on easily. You already argue in that direction

We would be breaking existing (experimental) API ...

Do you think this will get any better if done later?

[Tyriar]

AFAICT this would be the first inconsistency for properties being exposed as a function (except for maybe set/getOption but they have been around forever), it would be a shame if we stopped following the standard now and made it inconsistent as I've been told from several people about how xterm.js is easy to use.

I looked over #2369 again and thought about it, how about an optional argument that needs to be passed to calculate the flags/colors?

interface IBufferLine {
  /**
   * Gets a cell from the line, or undefined if the line index does not exist.
   *
   * Note that the result of this function should be used immediately after
   * calling as when the terminal updates it could lead to unexpected
   * behavior.
   * 
   * @param x The character index to get.
   * @param includeAttributes Optional boolean that indicates whether to
   * calculate and include attribute data (font style, colors) in the returned
   * object.
   * @param cell Optional cell object to recycle as the result of this call.
   * This can boost performance by reducing browser garbage collection time when
   * iterating over the entire buffer.
   */
  getCell(x: number, includeAttributes?: boolean, cell?: IBufferCell): IBufferCell | undefined;
}

interface IBufferCell {
  /**
   * The character within the cell.
   */
  readonly char: string;

  /**
   * The width of the character. Some examples:
   *
   * - This is `1` for most cells.
   * - This is `2` for wide character like CJK glyphs.
   * - This is `0` for cells immediately following cells with a width of `2`.
   */
  readonly width: number;

  /**
   * Text attribute flags like bold, underline etc.
   * 
   * Note that this should only be used when `includeAttributes` is passed to
   * `getCell`. Never store this object as it may get recycled by the library.
   */
  readonly flags: IBufferCellFlags;

  /**
   * Foreground color.
   * 
   * Note that this should only be used when `includeAttributes` is passed to
   * `getCell`. Never store this object as it may get recycled by the library.
   */
  readonly fg: IBufferCellColor;

  /**
   * Background color.
   * 
   * Note that this should only be used when `includeAttributes` is passed to
   * `getCell`. Never store this object as it may get recycled by the library.
   */
  readonly bg: IBufferCellColor;
}

So implementation would do this:

• With the optional cell? as in Add foreground/background color support for SerializeAddon #2369 we can keep GC down.
• The same recycling occurs in flags, fg and bg.
• Only when includeAttributes? is true will flags, fg or bg be valid, when false a shares invalid object could be used which throws with a good error message, for example (this object would be a shared constant):

  const invalidFg = Object.freeze({
    colorMode: () => throw new Error('You must call getCell with includeAttributes to use the fg property'),
    ...
  });
  fg = invalidFg;

This would allow us to keep the naming scheme as well as probably boost performance for cases like #2369 where we need all cell properties since we're dealing with simple object references/primitives rather than in-between functions.

[JavaCS3]

@Tyriar This one looks better since x is a constant value in a sense

class BufferCellApiView implements IBufferCellApi {
  public char: string;
  public width: number;
  constructor(line: IBufferLine, x: number) {
    this.char = line.getString(x);
    this.width = line.getWidth(x);
  }
}

[jerch]

@Tyriar How would the implementation look for the (yet to come) attributes with this? I played with a getter based variant in my playground branch: https://github.com/jerch/xterm.js/blob/09de17dbdb682ed3dbfac7dc3e504f1c28d89055/src/public/Terminal.ts#L238-L281

This variant tries to omit the recreation of subobjects for flags, fg and bg by closuring the .cell property and applying getters on that if a reference cell was given (otherwise all has to be created newly). Yet this shows the weird deopt problem. That was the point for me to say - ok, screw that approach, go with a much simpler linear thingy with access methods.

Also what @JavaCS3 points to might be a source of inconsistency - we have to decide whether a cell in the API should have stable content once requested (created/realized in memory) or always derive from line[x] offset (which might change content between write calls). We have discussed that already in an older thread, but kinda never resolved. If we stick with the line.getString(x) approach (deriving from line[x] offset), we have to extend the internal BufferLine API as well for the attributes to allow "hot loading" of the values.

Yepp, sticking with the optional cell argument will lift a big part of the runtime penalty already, still not sure how the includeAttributes flag will help here? Doesnt that shortcut the attribute processing, thus making any attempt with attributes expensive again? (Well this certainly depends on the actual implementation of the API attributes layer)

Edit: Maybe we should not try to to get an attribute API with v4 yet. To me this still seems to be to unreliable to be spec'ed out. How about just leaving it as it is for now (with experimental tag to indicate possible changes in the future)?

[Tyriar]

still not sure how the includeAttributes flag will help here? Doesnt that shortcut the attribute processing, thus making any attempt with attributes expensive again?

No, the idea was that it would work something like this:

const errorProp = () => throw new Error('You must call getCell with includeAttributes to use this');
// Pretty sure this doesn't compile but it gets the idea across
const invalidColor = { get colorMode(): errorProp, get color(): errorProp, get rgb(): errorProp};
const invalidFlags = ...;
...

constructor(x: number, includeAttributes?: boolean, cell?: IBufferCell) {
  cell = cell || ...;
  if (includeAttributes) {
    // Calculate and set attributes because the user wants them
  } else {
    cell.fg = invalidColor;
    cell.bg = invalidColor;
    cell.flags = invalidFlags;
  }
}

Any call to !includeAttributes would throw, so it shouldn't cause a deopt since cell always has a valid object of the same shape. I believe this I think we might even be able to use conditional types to error at compile time if we wanted to go down that route, especially if we run into problems getting rid of the deopt when throwing the exception.

Maybe we should not try to to get an attribute API with v4 yet.

Sounds better than a hasty decision 👍

[JavaCS3]

It looks like still have trouble finalizing API design.

[Tyriar]

@jerch thoughts on includeAttributes? Seems like a good compromise to me, keeping the existing naming structure by only fetching that data when it's requested.

[JavaCS3]

@jerch thoughts on includeAttributes? Seems like a good compromise to me, keeping the existing naming structure by only fetching that data when it's requested.

I don't like APIs using flags to change it's behavior except something like os.open(file, mode)

[jerch]

@Tyriar, @JavaCS3

I don't like APIs using flags to change it's behavior except something like os.open(file, mode)

Yeah I am not a big fan of this idea either, its like hiding 2 different APIs behind this flag (one with and one without attributes). And it does not really help with the question how to represent the attributes in the end.

Maybe we should stick with something like my interim proposal with sub-getters (this one), thus things dont add runtime if not requested (no additional costs if attributes are not needed), still the values can be lazy eval'ed (they are kinda at stand by).

Only thing that bugs me with this version is the deopt, that happens with 2+ run, but maybe we should ignore that for now and leave it to the JS engine devs to fix it in their JIT opt routines. Beside that deopt issue this version is still reasonable fast.

[JavaCS3]

@jerch What do u mean "deopt"? Sorry, I don't know this word.

[jerch]

@JavaCS3 I mean deoptimization with that.

[Tyriar]

Had a chat with @mofux who needs this API to be high performance. I think everyone is against me on the facade approach that adds extra protection to the API and makes all the calls consistent due to performance implications.

I think this is similar to what was originally exposed in @JavaCS3's PR but what about this:

interface IBufferLine {
    getCell(x: number, cell?: IBufferCell): IBufferCell | undefined;
}

interface IBufferCell {
  getWidth(): number;
  getChars(): string;
  getCode(): number;

  isInverse(): number;
  isBold(): number;
  isUnderline(): number;
  isBlink(): number;
  isInvisible(): number;
  isItalic(): number;
  isDim(): number;

  getFgColorMode(): number;
  getBgColorMode(): number;
  isFgRGB(): boolean;
  isBgRGB(): boolean;
  isFgPalette(): boolean;
  isBgPalette(): boolean;
  isFgDefault(): boolean;
  isBgDefault(): boolean;

  getFgColor(): number;
  getBgColor(): number;
}

[jerch]

@Tyriar Yeah this is most likely the fastest (not tested, but its kinda is a slim 1:1 abstraction of the internal structures, thus almost no API translation needed).

@ all
Would an API like that work for us? Have to admit that this API is not very JS/TS friendly shaped, esp. with all the color mode flags, which would only need 2 entries with an enum type (for fg/bg).

[mofux]

@jerch I agree that the huge amount of color functions could probably be shrunk down to a more simple to use color interface. In fact, the current interfaces are inappropriate to simply get the final color a cell would have. There is quite a lot of (duplicated) logic going on in our renderers that extract the correct color by considering the drawBoldTextInBrightColors option, as well as the inverse flag and the different color modes - as seen here:

xterm.js/src/browser/renderer/dom/DomRendererRowFactory.ts

Lines 100 to 126 in 5cb0bce

	const swapColor = this._workCell.isInverse();
	// fg
	if (this._workCell.isFgRGB()) {
	let style = charElement.getAttribute('style') || '';
	style += `${swapColor ? 'background-' : ''}color:rgb(${(AttributeData.toColorRGB(this._workCell.getFgColor())).join(',')});`;
	charElement.setAttribute('style', style);
	} else if (this._workCell.isFgPalette()) {
	let fg = this._workCell.getFgColor();
	if (this._workCell.isBold() && fg < 8 && !swapColor && this._optionsService.options.drawBoldTextInBrightColors) {
	fg += 8;
	}
	charElement.classList.add(`xterm-${swapColor ? 'b' : 'f'}g-${fg}`);
	} else if (swapColor) {
	charElement.classList.add(`xterm-bg-${INVERTED_DEFAULT_COLOR}`);
	}
	// bg
	if (this._workCell.isBgRGB()) {
	let style = charElement.getAttribute('style') || '';
	style += `${swapColor ? '' : 'background-'}color:rgb(${(AttributeData.toColorRGB(this._workCell.getBgColor())).join(',')});`;
	charElement.setAttribute('style', style);
	} else if (this._workCell.isBgPalette()) {
	charElement.classList.add(`xterm-${swapColor ? 'f' : 'b'}g-${this._workCell.getBgColor()}`);
	} else if (swapColor) {
	charElement.classList.add(`xterm-fg-${INVERTED_DEFAULT_COLOR}`);
	}

IMO the getFgColor() and getBgColor() interfaces should return a color object that is easier to work with, and that resolves the final fg / bg color (maybe as an [r,g,b] array?).

I'd also like the getCell(x: number): IBufferCell interface to offer the same (optional) recycle option that we have with the loadCell interface on our BufferLine by allowing to pass an optional existing IBufferCell as the second argument.

[Tyriar]

Yeah this is most likely the fastest (not tested, but its kinda is a slim 1:1 abstraction of the internal structures, thus almost no API translation needed).

It's actually just a subset of interfaces from AttributeData and CellData (most of the "getters").

IMO the getFgColor() and getBgColor() interfaces should return a color object that is easier to work with, and that resolves the final fg / bg color (maybe as an [r,g,b] array?).

While we could potentially expose it like this in the API, just to clarify we can't do it internally as the buffer shouldn't know about the theme. Trying to generalize this internally would likely be overly complicated.

Also for buffer serialization we definitely still need to expose the palette type.

[mofux]

While we could potentially expose it like this in the API

I think that would be good to have. Otherwise we would probably have to expose certain enums and functions that would be required to figure out the final color

we can't do it internally as the buffer shouldn't know about the theme

That's a very good point - I haven't considered that

[jerch]

I second that, imho theme related stuff should not be part of a "buffer API". If we want that at public API level some render related sub API might be better for this kind of data.

[Tyriar]

I added cell? to the above as we want recycling:

interface IBufferLine {
    getCell(x: number, cell?: IBufferCell): IBufferCell | undefined;
}

[mofux]

@jerch You're right, it's out of scope for a Buffer API. We can add additional API for getting the themed color values at some other place when requested.

[JavaCS3]

Had a chat with @mofux who needs this API to be high performance. I think everyone is against me on the facade approach that adds extra protection to the API and makes all the calls consistent due to performance implications.

I think this is similar to what was originally exposed in @JavaCS3's PR but what about this:

interface IBufferLine {
    getCell(x: number, cell?: IBufferCell): IBufferCell | undefined;
}

interface IBufferCell {
  getWidth(): number;
  getChars(): string;
  getCode(): number;

  isInverse(): number;
  isBold(): number;
  isUnderline(): number;
  isBlink(): number;
  isInvisible(): number;
  isItalic(): number;
  isDim(): number;

  getFgColorMode(): number;
  getBgColorMode(): number;
  isFgRGB(): boolean;
  isBgRGB(): boolean;
  isFgPalette(): boolean;
  isBgPalette(): boolean;
  isFgDefault(): boolean;
  isBgDefault(): boolean;

  getFgColor(): number;
  getBgColor(): number;
}

Is this the final API?

[Tyriar]

@JavaCS3 I think we've all decided on exposing the raw CellData object through the API but just restricting it via the interface. So if additional changes happen they will be very minimal only involve adding helpers to CellData or removing methods from IBufferCell.

@mofux @jerch any additional feedback on the API? Should we have helpers that convert the colors into a more useful object like IColor?

[jerch]

Im good with this, imho its the fastest/thinnest API we can get atm.