Add foreground/background color support for SerializeAddon #2369
Conversation
| @@ -4,9 +4,223 @@ | |||
| */ | |||
|
|
|||
| import { Terminal, ITerminalAddon } from 'xterm'; | |||
| import { ICellData } from 'common/Types'; | |||
| import { CellData } from 'common/buffer/CellData'; | |||
There was a problem hiding this comment.
Usage of anything in common/ is pulling in private API which will cause versioning issues for the addon down the track. Also note that this includes the contents of CellData.ts and Constants.ts inside the addon as well.
I think the right fix here is to expose the necessary attributes on IBufferCell in xterm.d.ts and commit to that as API.
src/public/Terminal.ts
Outdated
| public get width(): number { return this._line.getWidth(this._x); } | ||
| constructor(private _cell: ICellData) {} | ||
|
|
||
| public get fg(): number { return this._cell.fg; } |
There was a problem hiding this comment.
Sorry I didn't elaborate on the exact direction here but I think we need a slimmer API here for simplicity. ICellData is quite complex and just exposes a bunch of numbers and booleans so that the internals can be as fast as possible, but for the buffer API we assume the usage isn't going need that sort of level of optimization so we can put a nice simple facade on top. Here is what I'm thinking but I'm curious what @jerch thinks:
interface IBufferCell {
readonly char: string;
readonly width: number;
readonly foregroundColor: Color;
readonly backgroundColor: Color;
readonly style: Style;
}
type Color = IDefaultColor | IPalette16Color | IPalette256Color | IRgbColor;
type CellStyle = 'inverse' | 'bold' | 'underline' | 'blink' | 'invisible' | 'italic' | 'dim';
interface IDefaultColor {
type: 'default';
}
interface IRgbColor {
type: 'rgb';
// 0-255
red: number;
green: number;
blue: number;
}
interface IPalette16Color {
type: 'palette16';
// 0-15
id: number;
}
interface IPalette256Color {
type: 'palette256';
// 0-255
id: number;
}Note that we do use colors as strings elsewhere in the API but those are provided by the consumer which #rrggbb is a convenient format for, here we're exposing to consumers and Color which saves people from parsing that out. It does get a little complex with all the color modes but I'm not sure we can get simpler than that.
There was a problem hiding this comment.
Thanks for quick response. I have some concerns about the CellStyle
- There's a case a cell will have not only one style. For example,
\x1b[1;4mBoldAndUnderlined\x1b[0m. - Another thing is I need to make serialize result as small as possible. For example
If previous cell style is Bold and Underline the next cell is Bold. I only need to unset Underline flag by\x1b[24minstead of always reset to\x1b[0mfor each cell. That's why I have XOR caculation which bit of Style changed.
So I think bitwise based style is more efficient. Maybe you have better way to solve this problem.
There was a problem hiding this comment.
@Tyriar, @JavaCS3:
I second what @JavaCS3 said, maybe we can keep the bitflags for CellStyle, but merged from BgFlags and FgFlags into one:
enum CellStyle {
// first byte FgFlags
bold = ... (must match bit pos in FgFlags + conversion)
// second byte BgFlags
...
};
// conversion in API getter:
public get style(): CellStyle {
// maybe cache the result
return (this._cell.bg >> 16) | (this._cell.fg >> 24);
}
// boolean eval in addon
if (style & CellStyle.BOLD) ...About IColor:
The type member on the color types is important to avoid costly instanceof checks, yeah.
Offtopic but relevant in the bigger API picture:
Problem with the color types and number exposed here I see is that ppl might not be interested in this pre-render stage data but want to grab the color thats going to be shown. Note that this is basically the same issue as I pointed out in the previous PR about empty cells vs. whitespace (just the way around now). I think we have to make clear what we want to expose in BufferApi - shall this be a buffer data representation layer or more in line with what the renderer would make of it?
Proposal to solve this ambiguity: Make BufferApi what the name suggests, expose the data from the buffer, thus with different color types (that have no meaning without a theme) and WITH empty cells by default.
Later on in a second API expose things that would be applied on the way to the screen, like right trimming / whitespace replacements and colors from the active theme.
There was a problem hiding this comment.
👍 to flags for cell style.
Later on in a second API expose things that would be applied on the way to the screen, like right trimming / whitespace replacements and colors from the active theme.
Not sure we want to go in that direction, the current method of exposing the different color types works for @JavaCS3 and my use case as its purely for serializing and restoring.
There was a problem hiding this comment.
@Tyriar Well to me it seems kinda arbitrary why we expose buffer content with different rules applied here:
- char content: Gets the trimming and the whitespace replacements as the renderer would do, thus we actually dont expose buffer content here
- colors: Exposes buffer content with no renderer stuff applied
Imho thats confusing for a "buffer" API. Btw this will get even more confusing once we support bidi and such. Which one we gonna expose here then for char content?
There was a problem hiding this comment.
Should I go ahead with @jerch 's proposal or wait until the BufferAPI to be finialized?
There was a problem hiding this comment.
Gets the trimming and the whitespace replacements as the renderer would do, thus we actually dont expose buffer content here
Not sure I understand, we're returning the character or '' for char:
xterm.js/src/public/Terminal.ts
Line 217 in 95ff154
If you request a column that is "null" whitespace on the right of a column wouldn't it would return as ''?
There was a problem hiding this comment.
Ah sorry thought you are working with translateToString 😅 , getString correctly returns an empty string for empty cells. So on individual cell level you get the right value, all good.
There was a problem hiding this comment.
I didn't meet any trouble using the exisiting public get char(): string.
My latest change NullBufferCell may mislead u guys. The main reason why I do that is I want to make changes small. There's a case (_bgChanged(...) function) I need to tell the previous cell of a cell. But when it's the first cell, the previous cell is not even exisit. So I just use a "NullObject Strategy" to solve it in a quick way. It doesn't mean I need to treat null cell differently.
Maybe later on, I will meet some kind of null cell problems. We will see. For now it's ok to me.
typings/xterm.d.ts
Outdated
|
|
||
| interface IDefaultColor extends ICellColor { | ||
| type: 'default'; | ||
| hash: 0; |
There was a problem hiding this comment.
hash is designed to quickly tell if two color is different.
| import { Terminal, ITerminalAddon, IBuffer, IBufferCell, Color } from 'xterm'; | ||
|
|
||
| // TODO: Workaround here, will remove this later | ||
| // If I use `import { CellStyle } from 'xterm'` instead, demo page will raise bellow error |
There was a problem hiding this comment.
I don't know how to solve it in a quick way. I just wanna give u guys a glance of using the new API proposal.
There was a problem hiding this comment.
I think the issue you're seeing is you're mixing types and implementation. xterm.d.ts should not contain the implementation of the enum, only the contract. Try moving all the = values out into Terminal.ts. I don't think we've donee this with enums yet but I expect it to work.
|
Please check this new update |
src/public/Terminal.ts
Outdated
| case Attributes.CM_P256: return { type: 'palette256', hash, id: cell.getFgColor() }; | ||
| } | ||
| } else if (cell.isFgRGB()) { | ||
| const [red, green, blue] = CellData.toColorRGB(cell.fg); |
There was a problem hiding this comment.
Just curious - how does this perform if you serialize tons of lines (>10000)? Note that the interim creation of objects is quite expensive, basically for every cell 2 color objects have to be created and for RGB even 4 (due to the interim [r,g,b] array).
You basically hit the same problem here I had when I designed the buffer cells access. I went with the "ref style" approach with loadCell to avoid these costs in renderers. In my benchmarks the interim objects were about 3-4 times slower.
Not sure if we could resemble this to some degree here (not even sure if its worth to try, thus the question about the performance).
Edit: Additional sidenote to this - the color types now will change their "object shapes" with different properties depending on being RGB or not. Thats often a reason for the v8 optimizer to bail out - which can lead to >5 times less performant code, might be better to shape them all the same way and just dont use the unwanted properties.
There was a problem hiding this comment.
I don't have benchmark yet. I think we could refactor it later if needed. There's a saying "premature optimization is the root of all evil". So I think we can optimize performance later after this feature is working.
There was a problem hiding this comment.
Well if you cite Donald Knuth you should also grasp the context where he explains this, which might not be true here: Its about hunting for small efficiency gains, which tend to eat tons of time and distract programmers. I suspect this to run at least 5 times slower, thats not what I'd call "small". Furthermore it might have direct impact on the API design which cannot be undone easily afterwards.
There was a problem hiding this comment.
I this case, you might be right. I have no idea how to improve it currently other than hex based algorithm do u have any design suggestions?
There was a problem hiding this comment.
No not yet.
Maybe try to benchmark your current version and another one which uses the private loadCell semantic to get a first impression. If thats within a factor of 2 I would'nt care much (its expected to lose some performance due to a stable API layer). If its bigger we might have to borrow some ideas from the internal way.
To only test the impact of the API layer with interim object creation its important that you dont hide the difference behind much more expensive code down the road (basically make .serialize a NOOP). You can either use our benchmark tool for it (see other benchmarks under /test/benchmark) or simply do some Date.now() debugging (less accurate but enough to grasp the difference if the amount of cells processed is high enough).
Edit: Just to give you some numbers: Removing interim object creation (and later deletion) was part of the big refactoring of the whole terminal input chain and lowered the GC pressure from ~20% to <1%. Overall gain (with more changes all over the code): throughput went from ~7MB/s up to ~35MB/s.
There was a problem hiding this comment.
@jerch You're right the new design performance is worser than the original one.
Notes: because I need to serialize color, so I use lolcat to help me generate a colorful output
Highlights:
- Private API: average throughput: 8.03 MB/s
- The New API: average throughput: 2.51 MB/s
I have pushed my comparison code into my personal repo. you can check it by this link
I think the key difference is the Color schema is dynamic and need to be created for each cell. But the original private api still need to new CellData() for each cell as well. Below is the details.
# Private API
➜ xterm.js yarn benchmark-baseline
yarn run v1.17.3
$ NODE_PATH=./out:./out-test/benchmark xterm-benchmark -r 5 -c test/benchmark/benchmark.json --baseline out-test/benchmark/test/benchmark/SerializeAddon.benchmark.js
Context "out-test/benchmark/test/benchmark/SerializeAddon.benchmark.js"
Context "Terminal: sh -c "ls -lR /usr/lib | lolcat -f""
Context "serialize"
serialize addon: startRow= 0 endRow= 706
serialize addon: startRow= 0 endRow= 706
serialize addon: startRow= 0 endRow= 706
serialize addon: startRow= 0 endRow= 706
serialize addon: startRow= 0 endRow= 706
Case "#1" : 5 runs - average throughput: 8.03 MB/s
### Baseline data ###
"out-test/benchmark/test/benchmark/SerializeAddon.benchmark.js|Terminal: sh -c "ls -lR /usr/lib | lolcat -f"|serialize|#1"
#averageRuntime
STAT BASE TOLERANCE
mean 18.77 0.75-1.50
median 18.71 skipped
dev 12.16 skipped
cv 0.65 skipped
runs 5.00 skipped
#averageThroughput
STAT BASE TOLERANCE
mean 8.03 0.75-1.50
median 5.22 skipped
dev 6.01 skipped
cv 0.75 skipped
runs 5.00 skipped
✨ Done in 4.57s.
# The New API
➜ xterm.js yarn benchmark-eval
yarn run v1.17.3
$ NODE_PATH=./out:./out-test/benchmark xterm-benchmark -r 5 -c test/benchmark/benchmark.json --eval out-test/benchmark/test/benchmark/SerializeAddon.benchmark.js
Context "out-test/benchmark/test/benchmark/SerializeAddon.benchmark.js"
Context "Terminal: sh -c "ls -lR /usr/lib | lolcat -f""
Context "serialize"
serialize addon: startRow= 0 endRow= 706
serialize addon: startRow= 0 endRow= 706
serialize addon: startRow= 0 endRow= 706
serialize addon: startRow= 0 endRow= 706
serialize addon: startRow= 0 endRow= 706
Case "#1" : 5 runs - average throughput: 2.51 MB/s
"out-test/benchmark/test/benchmark/SerializeAddon.benchmark.js|Terminal: sh -c "ls -lR /usr/lib | lolcat -f"|serialize|#1"
#averageRuntime
STAT BASE TOLERANCE VALUE CHANGE(%) EVAL
mean 18.77 0.75-1.50 40.53 115.96 FAILED
median 18.71 skipped 39.43 SKIP
dev 12.16 skipped 8.87 SKIP
cv 0.65 skipped 0.22 SKIP
runs 5.00 skipped 5.00 SKIP
#averageThroughput
STAT BASE TOLERANCE VALUE CHANGE(%) EVAL
mean 8.03 0.75-1.50 2.51 -68.77 FAILED
median 5.22 skipped 2.48 SKIP
dev 6.01 skipped 0.54 SKIP
cv 0.75 skipped 0.22 SKIP
runs 5.00 skipped 5.00 SKIP
Success: 0
Missing: 0
Skipped: 8
Failed: 2
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.There was a problem hiding this comment.
Wow, thanks for benchmarking it this extensively 👍. The numbers are within what I expected, hmm. About the new CellData() - yeah the renderers even skip that by reusing the same object over and over and just load the primitive numbers with loadCell.
Geez, seems this will be a tradeoff between a nice and easy to use API and performance. I dont want to be to picky here about performance, so maybe we can find some middle ground to shape the API.
Still have to go through your benchmark tests, will see if I can come up with some alternative API suggestions. Feel free to do so as well.
There was a problem hiding this comment.
It doesn't seem that bad considering this will happen so infrequently, making the API easy to use is pretty important.
There was a problem hiding this comment.
Worst case estimation:
Given a fullscreen terminal with 400 cols x 10k scrollback serialize would have to process 4M cells. If every cell contains different RGB foreground and RGB background a cell string would end up as a string of length ~40, thus total runtime is about:
4M * 40 / 2.5 = 64s
4M * 40 / 8 = 20s
The values are extreme values by purpose, a typical terminal instance would be more in the range of 120 cols x 1k scrollback with most cells containing colors of their neighbors (the string contains only ~2 chars on average), thus the typical runtime might be more like:
120k * 2 / 2.5 = 96ms
120k * 2 / 8 = 30ms
With 8MB/s we are right at the perceivable delay threshold, with 2.5 MB/s we have a small hiccup. Note that this will freeze everything else within a bigger application/page for that amount of time, so I am not sure if thats bearable. I see two possible solutions for that:
- stick with the slower API design, but move
.serializeand needed buffer data to a webworker - extend the API with an optional ref style way, maybe we just need to add a
getCell(cell?: BufferCell)method, that can optionally copy stuff into a given cell instead of creating new objects and keeping the GC busy (also omitting the recreation of the cell itself, which should give us rates >15MB/s) - third way: do both 😸
There was a problem hiding this comment.
webworker is overkill to me. I even want to run xterm.js on server side. I'm ok even if the API doesn't looks pretty, I think 2nd looks better.
How about combine palette16/256/RGB into one single data structure? @jerch @Tyriar
interface ICellColor {
type: 'default' | 'rgb' | 'palette16' | 'palette256';
hash: number;
id: number;
red: number;
green: number;
yellow: number;
}or
interface ICellColor {
type: 'default' | 'rgb' | 'palette16' | 'palette256';
hash: number;
equals(c: ICellColor): boolean;
toPaletteId(): number;
toRGBColor(): [number, number, number];
}
| import { Terminal, ITerminalAddon, IBuffer, IBufferCell, Color } from 'xterm'; | ||
|
|
||
| // TODO: Workaround here, will remove this later | ||
| // If I use `import { CellStyle } from 'xterm'` instead, demo page will raise bellow error |
There was a problem hiding this comment.
I think the issue you're seeing is you're mixing types and implementation. xterm.d.ts should not contain the implementation of the enum, only the contract. Try moving all the = values out into Terminal.ts. I don't think we've donee this with enums yet but I expect it to work.
typings/xterm.d.ts
Outdated
| } | ||
|
|
||
| interface ICellColor { | ||
| type: string; |
There was a problem hiding this comment.
You might need to remove type here? Ideally we want it to error when you try to do something like this:
if (color.type === 'fake')
If you have it declared as string it might become this:
type: 'default' | 'rgb' | 'palette16' | 'palette256' | string;
We want it to be this:
type: 'default' | 'rgb' | 'palette16' | 'palette256';
typings/xterm.d.ts
Outdated
|
|
||
| interface IDefaultColor extends ICellColor { | ||
| type: 'default'; | ||
| hash: 0; |
There was a problem hiding this comment.
I think you can safely remove hash from all the extended interfaces, declaring it as 0/static here probably doesn't give us anything?
Tyriar
changed the title
|
The latest change still have trouble compiling demo page with the following error, I need some help. |
|
@JavaCS3 I encounter similar errors after switching branches. For some reason the watcher gets often stuck by this. My solution then is to remove all builds (all Sorry for not answering on the topic above, hope to get back on track by the weekend. |
@jerch My error is not related to // ...
Object.defineProperty(exports, "__esModule", { value: true });
var xterm_1 = require("xterm");
var CellStyle;
// ... |
|
@jerch I accidently fixed that problem after read webpack docs and google. Hope this is the right way to solve it. |
|
@jerch How about this now? I run another benchmark and workaround some imporing issues to get this new benchmark. Looks like a little improve than the previous one. |
|
@JavaCS3 Yeah thats much better. Sadly I cannot try myself - cannot get it working prolly due to the same error as you had earlier (missing 'xterm' stuff). If you have some pointers how to set it up correctly I could have a look as well. I still think trying a ref style cell thingy on API level might be worth a shot, I expect this to run at +15 MB/s rates (I still see 35 MB/s for the input benchmark with the lolcat command, note that the input chain does alot more like UTF32 conversion, parsing, calling target commands, insert to terminal buffer). Note that at higher rates serialize might suffer from a string concat performance bottleneck, to level this out you might have to play with different string creation semantics (e.g. Edit: with ref style thingy I mean something like this: const cell1 = new term.<somewhere on the API>.Cell(); // cell from exported cell ctor
const cell2 = new term.<somewhere on the API>.Cell();
// use it in serialize
for (<all cells to process>) {
...
// loads cell content into cell obj thus avoid recreation
// the cell argument could be optional, thus if not provided
// a new obj is created by default (otherwise newCell === cell1 or cell2)
const newCell = term.buffer.getLine(y).getCell(x, oldCell === cell1 ? cell2 : cell1);
...
// later on do clever flipping of old cell and new cell to avoid copies
// and just compare the contents
if (newCell.attrX !== oldCell.attrX) {
// insert some CSI codes to output to reflect differences
...
oldCell = newCell; // reassign oldCell if there were differences
}
...
}Ofc this needs further specs of the Cell thingy on the API. Imho thats the point where the performance grail can be found. It boils down to 2 object creations (cell1 and cell2) + the string concat for the output, everything else is just number copying and evaluating differences - basically sending malloc + GC on vacation. |
|
@jerch If you need to run benchmark on the latest code, I did some tricks
diff --git a/test/benchmark/tsconfig.json b/test/benchmark/tsconfig.json
index 51abeba5..60c6bcc1 100644
--- a/test/benchmark/tsconfig.json
+++ b/test/benchmark/tsconfig.json
@@ -18,7 +18,8 @@
"browser/*": [ "../../src/browser/*" ],
"addons/xterm-addon-serialize/src/*": ["../../addons/xterm-addon-serialize/src/*"],
"public/*": ["../../src/public/*"],
- "Terminal": ["../../src/Terminal"]
+ "Terminal": ["../../src/Terminal"],
+ "xterm": ["../../src/public/Terminal"]
},
},
"include": [
The key point of "missing 'xterm' stuff" is the compilation output of |
|
@JavaCS3 Thx, got it working (with a few more changes though). These are the results for my machine (serializing 80x1000 lines of lolcat): I switched on the reports for individual runs - as I already suspected by your mean/median and cv values from above they differ alot and are badly skewed. Since it shows much better runtime in the last runs its most likely due to some late optimizations done by v8. |
|
@JavaCS3 Hacked quickly a version using private APIs with ref style, see https://gist.github.com/jerch/2dfaea9b52f8573456894dad70fa0f1c. Benchmark results with same test file: It still shows that weird speedup, but much earlier this time. I did not optimize anything in the code, it is mostly c&p from core stuff. I think we should rethink the public API from here, not the way around. 17 MB/s vs. 4 MB/s is quite alot. Edit: If I repeat the whole buffer serialize in the test multiple times it converges towards 20 MB/s. |
|
Proposal for public API: diff --git a/src/public/Terminal.ts b/src/public/Terminal.ts
index 6d6cedca..f63b926b 100644
--- a/src/public/Terminal.ts
+++ b/src/public/Terminal.ts
@@ -3,7 +3,7 @@
* @license MIT
*/
-import { Terminal as ITerminalApi, ITerminalOptions, IMarker, IDisposable, ILinkMatcherOptions, ITheme, ILocalizableStrings, ITerminalAddon, ISelectionPosition, IBuffer as IBufferApi, IBufferLine as IBufferLineApi, IBufferCell as IBufferCellApi, IParser, IFunctionIdentifier } from 'xterm';
+import { Terminal as ITerminalApi, ITerminalOptions, IMarker, IDisposable, ILinkMatcherOptions, ITheme, ILocalizableStrings, ITerminalAddon, ISelectionPosition, IBuffer as IBufferApi, IBufferLine as IBufferLineApi, IBufferCell as IBufferCellApi, IParser, IFunctionIdentifier, IBufferCellColor as IBufferCellColorApi } from 'xterm';
import { ITerminal } from '../Types';
import { IBufferLine } from 'common/Types';
import { IBuffer } from 'common/buffer/Types';
@@ -12,6 +12,9 @@ import * as Strings from '../browser/LocalizableStrings';
import { IEvent } from 'common/EventEmitter';
import { AddonManager } from './AddonManager';
import { IParams } from 'common/parser/Types';
+import { CellData } from '../../out/common/buffer/CellData';
+import { Attributes } from '../../out/common/buffer/Constants';
+import { AttributeData } from '../../out/common/buffer/AttributeData';
export class Terminal implements ITerminalApi {
private _core: ITerminal;
@@ -199,17 +202,24 @@ class BufferApiView implements IBufferApi {
}
return new BufferLineApiView(line);
}
+ public getNullCell(): IBufferCellApi {
+ return new BufferCellApiView(new CellData());
+ }
}
class BufferLineApiView implements IBufferLineApi {
constructor(private _line: IBufferLine) {}
public get isWrapped(): boolean { return this._line.isWrapped; }
- public getCell(x: number): IBufferCellApi | undefined {
+ public getCell(x: number, cell?: BufferCellApiView): IBufferCellApi | undefined {
if (x < 0 || x >= this._line.length) {
return undefined;
}
- return new BufferCellApiView(this._line, x);
+ if (cell) {
+ this._line.loadCell(x, cell.cell);
+ return cell;
+ }
+ return new BufferCellApiView(this._line.loadCell(x, new CellData()));
}
public translateToString(trimRight?: boolean, startColumn?: number, endColumn?: number): string {
return this._line.translateToString(trimRight, startColumn, endColumn);
@@ -217,9 +227,42 @@ class BufferLineApiView implements IBufferLineApi {
}
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); }
+ public flags: {[flag: string]: boolean};
+ public fg: IBufferCellColorApi;
+ public bg: IBufferCellColorApi;
+ constructor(public cell: CellData) {
+ this.flags = {
+ get bold(): boolean { return !!cell.isBold(); },
+ get underline(): boolean { return !!cell.isUnderline(); },
+ get blink(): boolean { return !!cell.isBlink(); },
+ get inverse(): boolean { return !!cell.isInverse(); },
+ get invisible(): boolean { return !!cell.isInvisible(); },
+ get italic(): boolean { return !!cell.isItalic(); },
+ get dim(): boolean { return !!cell.isDim(); }
+ };
+ this.fg = {
+ get colorMode(): "RGB" | "P256" | "P16" | "DEFAULT" {
+ return cell.getFgColorMode() === Attributes.CM_RGB ? 'RGB'
+ : cell.getFgColorMode() === Attributes.CM_P256 ? 'P256'
+ : cell.getFgColorMode() === Attributes.CM_P16 ? 'P16'
+ : 'DEFAULT'
+ },
+ get color(): number { return cell.getFgColor(); },
+ get rgb(): number[] { return AttributeData.toColorRGB(cell.getFgColor()); }
+ };
+ this.bg = {
+ get colorMode(): "RGB" | "P256" | "P16" | "DEFAULT" {
+ return cell.getBgColorMode() === Attributes.CM_RGB ? 'RGB'
+ : cell.getBgColorMode() === Attributes.CM_P256 ? 'P256'
+ : cell.getBgColorMode() === Attributes.CM_P16 ? 'P16'
+ : 'DEFAULT'
+ },
+ get color(): number { return cell.getBgColor(); },
+ get rgb(): number[] { return AttributeData.toColorRGB(cell.getBgColor()); }
+ };
+ }
+ public get char(): string { return this.cell.getChars(); }
+ public get width(): number { return this.cell.getWidth(); }
}
class ParserApi implements IParser {
diff --git a/typings/xterm.d.ts b/typings/xterm.d.ts
index cae54dc1..43c8fc0b 100644
--- a/typings/xterm.d.ts
+++ b/typings/xterm.d.ts
@@ -878,6 +878,13 @@ declare module 'xterm' {
* @param y The line index to get.
*/
getLine(y: number): IBufferLine | undefined;
+
+ /**
+ * Creates an empty cell object suitable as a cell reference in
+ * `line.getCell(x, cell)`. Use this to avoid costly recreation of
+ * cell objects when dealing with tons of cells.
+ */
+ getNullCell(): IBufferCell;
}
/**
@@ -892,13 +899,10 @@ declare module 'xterm' {
/**
* 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 cell Optional cell object to load data into.
*/
- getCell(x: number): IBufferCell | undefined;
+ getCell(x: number, cell?: IBufferCell): IBufferCell | undefined;
/**
* Gets the line as a string. Note that this is gets only the string for the
@@ -911,6 +915,36 @@ declare module 'xterm' {
translateToString(trimRight?: boolean, startColumn?: number, endColumn?: number): string;
}
+ /**
+ * Represents foreground and background color settings of a cell.
+ */
+ interface IBufferCellColor {
+ /**
+ * Color mode of the color setting.
+ * RGB Color is an RGB color, use `.rgb` to grab the different channels.
+ * P256 Color is an indexed value of the 256 color palette.
+ * P16 Color is an indexed value of the 8 color palette (+8 for AIX bright colors).
+ * DEFAULT No color set, thus default color should be used.
+ */
+ colorMode: 'RGB' | 'P256' | 'P16' | 'DEFAULT';
+
+ /**
+ * Color value set in the current color mode.
+ * Note that the color value can only be interpreted in conjunction
+ * with the color mode:
+ * RGB color contains 8 bit channels in RGB32 bitorder, e.g. red << 16 | green << 8 | blue
+ * P256 color contains indexed value 0..255
+ * P16 color contains indexed value 0..15
+ * DEFAULT color always contains -1
+ */
+ color: number;
+
+ /**
+ * Helper to get RGB channels from color mode RGB. Reports channels as [red, green, blue].
+ */
+ rgb: number[];
+ }
+
/**
* Represents a single cell in the terminal's buffer.
*/
@@ -928,6 +962,21 @@ declare module 'xterm' {
* - This is `0` for cells immediately following cells with a width of `2`.
*/
readonly width: number;
+
+ /**
+ * Text attribute flags like bold, underline etc.
+ */
+ readonly flags: {[flag: string]: boolean};
+
+ /**
+ * Foreground color.
+ */
+ readonly fg: IBufferCellColor;
+
+ /**
+ * Background color.
+ */
+ readonly bg: IBufferCellColor;
}
/**Changes:
This API approach tries to be easy to use while still resembling some parts of the C-ish internals for performance reasons. Main pro here is the possibility to reuse a previous cell object, which will greatly lower the allocation + GC pressure. Not yet benchmarked though... |
|
The new API works, but the needed lazy evaluation of the different attributes makes it still slow (~5MB/s). Thus I added a few convenient compare methods that lift some burden from the long if cascades in serialize: // public API in xterm.d.ts
/**
* Whether cells have the same text attributes (flags and colors).
* @param other Other cell.
*/
equalAttibutes(other: IBufferCell): boolean;
/**
* Whether cells have the same text attribute flags.
* @param other Other cell.
*/
equalFlags(other: IBufferCell): boolean;
/**
* Whether cells have the same foreground color.
* @param other Other cell.
*/
equalFg(other: IBufferCell): boolean;
/**
* Whether cells have the same background color.
* @param other Other cell.
*/
equalBg(other: IBufferCell): boolean;
// impl in src/public/Terminal.ts
public equalAttibutes(other: BufferCellApiView): boolean {
return this.cell.fg === other.cell.fg
&& this.cell.bg === other.cell.bg;
}
public equalFlags(other: BufferCellApiView): boolean {
return (this.cell.fg & fgFlagMask) === (other.cell.fg & fgFlagMask)
&& (this.cell.bg & bgFlagMask) === (other.cell.bg & bgFlagMask);
}
public equalFg(other: BufferCellApiView): boolean {
return this.cell.getFgColorMode() === other.cell.getFgColorMode()
&& this.cell.getFgColor() === other.cell.getFgColor();
}
public equalBg(other: BufferCellApiView): boolean {
return this.cell.getBgColorMode() === other.cell.getBgColorMode()
&& this.cell.getBgColor() === other.cell.getBgColor();
}With these additions we are back on the speedy track: Source of my serialize implementation can be found here: https://gist.github.com/jerch/cfe4360e7f23df8bc7189c3761bd13f5 (Note that this is still not optiomized nor does it handle all weird circumstances.) Now thats pretty close to the complicated private C-ish access (15 MB/s vs. 20 MB/s). Seems to be a good compromise between performance and easy to use API. @JavaCS3: Maybe you can test your implementation with my API changes and see if the interfaces work for you? |
|
Made a direct comparison of private access vs. public API: The test was done with more typical The public API sacrifice is 20 - 25% in performance, which is imho good for the gains in simplicity (the API is much more JS-ish compared to the low level C-ish private stuff). Still the overhead for the getters is significant, The code can be viewed in my playground branch https://github.com/jerch/xterm.js/tree/serialize_with_private. I suggest to go with these API changes for the colors, as they can keep things speedy while still being simple to use. Maybe |
|
@jerch I'm trying to integrate your proposal. Off topic: |
Yes that would be nice. Imho the addon could have several "renderers" later on to select from like |
|
@jerch How to get that perf picture? |
|
@JavaCS3 Did that with devtools in the demo:
Edit: The high amount of scrollback was needed to get some resolution into subcalls, otherwise the sampling profiler will not spot enough details. Anything below 5ms is not very accurate. (Also make sure to have the sampling rate set to high in devtools.) |
|
@JavaCS3 Oh btw, forgot to test your flag copying with my API changes. If you care enough you can test copying flags over as well, it might squeeze a few cycles out of the flag access (mainly the |
|
@jerch I just found we can use |
|
@JavaCS3 Have never used this package, if you get it working that would be really nice. With xterm-benchmark you can switch on a debugger break and attach devtools by doing this:
If you really want to dive into this stuff I recommend reading through this article: https://nodesource.com/blog/why-the-new-v8-is-so-damn-fast/. Esp. the described |
|
@JavaCS3 Just found out that I had installed an old version of lolcat not capable to output truecolor. After installing as gem and switching truecolor on (-t switch), I get worse results with the current API. Seems the weird fg/bg objects with closured |
|
Pushed a new API version. Main changes:
The changes were needed due to my tests with truecolor cells created by lolcat. The old API with its closured getters got really slow after one or two rounds for no obvious reason - while the first invocation finishs after ~1200ms any later run takes 3000 - 4000 ms with the same data :(
@JavaCS3 Sorry for the constant API changes, but I think this is now in a pretty good shape and working more reliable. @Tyriar It seems the getters are still not a good idea when it comes to hot code. I think we should deprecate the |
Tyriar
force-pushed
the
feat/serialize-addon
branch
from
8f19506
to
ca12cf5
Compare
src/public/Terminal.ts
Outdated
| public isFgPalette16(): boolean { return this.cell.getFgColorMode() === Attributes.CM_P16; } | ||
| public isBgPalette16(): boolean { return this.cell.getBgColorMode() === Attributes.CM_P16; } | ||
| public isFgPalette256(): boolean { return this.cell.getFgColorMode() === Attributes.CM_P256; } | ||
| public isBgPalette256(): boolean { return this.cell.getBgColorMode() === Attributes.CM_P256; } |
There was a problem hiding this comment.
I don't think we need this differentiation, can't we just check if color is < 16 in Serialize Addon here?
else if (cell.isFgPalette256()) { sgrSeq.push(38, 5, color); }
else if (cell.isFgPalette16()) { sgrSeq.push(color & 8 ? 90 + (color & 7) : 30 + (color & 7)); }to:
else if (cell.isFgPalette()) {
if (color >= 16) {
sgrSeq.push(38, 5, color); }
} else {
sgrSeq.push(color & 8 ? 90 + (color & 7) : 30 + (color & 7));
}
}There was a problem hiding this comment.
@Tyriar When I was trying to follow your suggestion I found a problem. Basically, Color palette256 contains palette16.
So in my test case "serialize all rows of content with color256", your input \x1b[38;5;0m will be serialized into \x1b[30m instead which is a little bit different from the original color.
I don't know if the color code 0-16 in palette256 is equal to the color in palette16 all the time.
My test case is designed to make serialize output as closer as the original input.
What's your opinion?
And @jerch
There was a problem hiding this comment.
@JavaCS3 Imho you cannot assume the lower 16 colors of P256 being the same as the P16 colors. ECMA-48/ITU-T.416 do not state any linkage between those palettes, guess it should be treated as implementation/emulator dependent (I think some emulators short-circuit those colors internally).
This becomes interesting for theming - we currently only support theming of the P16 colors, here it seems arguable whether to apply those to the lower P256 colors as well (we currently dont and I think it should stay that way). Also xterm supports custom colors for P256 to be set via an OSC command (we dont support this yet), I think those would not change the P8/P16 likewise (have not tested it).
Edit: Seems xterm handles the OSC commands like this:
The color numbers correspond to the ANSI colors 0-7, their
bright versions 8-15, and if supported, the remainder of the
88-color or 256-color table.
Thus its not possible to address the lower 16 colors in P256 (as it always maps to P16).
src/public/Terminal.ts
Outdated
| public equalFlags(other: BufferCellApiView): boolean { | ||
| return (this.cell.fg & FG_FLAG_MASK) === (other.cell.fg & FG_FLAG_MASK) | ||
| && (this.cell.bg & BG_FLAG_MASK) === (other.cell.bg & BG_FLAG_MASK); | ||
| } | ||
| public equalFg(other: BufferCellApiView): boolean { | ||
| return (this.cell.fg & COLOR_MASK) === (other.cell.fg & COLOR_MASK); | ||
| } | ||
| public equalBg(other: BufferCellApiView): boolean { | ||
| return (this.cell.bg & COLOR_MASK) === (other.cell.bg & COLOR_MASK); | ||
| } |
There was a problem hiding this comment.
Instead of adding these can we just compare color mode/color and compare flags using isUnderline, etc.? I wouldn't this that would impact performance that much.
…ed(), _lineStart() functions
…api from xterm.d.ts
JavaCS3
force-pushed
the
feat/serialize-addon
branch
from
d1011cf
to
7a8c3a4
Compare
|
@Tyriar Here's the new update |
…ns/xterm-addon-serialize and add README.md
|
@Tyriar mission complete |
Tyriar
force-pushed
the
feat/serialize-addon
branch
from
3157068
to
a4efefb
Compare
src/common/buffer/Constants.ts
Outdated
| /** | ||
| * bit 27..31 (32th bit unused) | ||
| */ | ||
| FM_MASK = 0x7C000000 |
There was a problem hiding this comment.
These FM_MASK values aren't referenced anywhere anymore
typings/xterm.d.ts
Outdated
| @@ -1011,6 +1020,35 @@ declare module 'xterm' { | |||
| * - This is `0` for cells immediately following cells with a width of `2`. | |||
| */ | |||
| readonly width: number; | |||
There was a problem hiding this comment.
Let's add these 2 messages for char and width:
* @deprecated use `getChars` instead
* @deprecated use `getWidth instead
There was a problem hiding this comment.
@Tyriar I need to either remove these interface in xterm.d.ts because ICellData doesn't have width, char interface if you want to remove BufferCellApiView OR add width, char interface in ICellData and fix all the implementations of ICellData.
There was a problem hiding this comment.
Good point, let's remove them then since Terminal.buffer is marked experimental
src/public/Terminal.ts
Outdated
| this._line.loadCell(x, cell.cell); | ||
| return cell; | ||
| } | ||
| return new BufferCellApiView(this._line.loadCell(x, new CellData())); |
There was a problem hiding this comment.
This needs to change to be just:
return this._line.loadCell(x, new CellData());In order to get all the perf savings we're after, no point wrapping the object if they're not essentially equivalent. From what @jerch said earlier I think we can just check the color value when the mode is palette inside the addon instead of adding a new method to the API:
Thus its not possible to address the lower 16 colors in P256 (as it always maps to P16).
There was a problem hiding this comment.
@Tyriar See the latest update
|
@Tyriar Please check the latest update |
Fixes #2383
@Tyriar Here's the new update
Currently there're some un-implemented features: re-position cursor, wrap mode, combinedData
Will continue improve it.