Feature/xterm addon serialize restore layout and cursor

[mmis1000]

This pull request.

• Fix xterm-addon-serialize does not preserve empty cell background #3098
• Fix xterm-addon-serialize only serialize the alt screen if the terminal is in alt mode #3090
• Implement cursor position restoration
• Handles isWrapped property
• changes the rows option to scrollback to make it work with cursor restoration.
• Add a test util to xterm-addon-serialize that export the screen buffer state as plain object for testing.

Tested on a few full screen console programs to make sure the layout is identical to original state.

• htop
• vim
• nano
• sl

[mmis1000]

WTF, how do the same bit computation result in -0 instead of 0 in firefox?

[mofux]

I didn't even know this was possible. 0 and even Infinity can be signed!
And guess what, while -0 === 0, -Infinity !== Infinity:

[mmis1000]

It looks like either xtermjs did something undefined behavior during computation (but i think js seldom do that?), or the firefox implement js incorrectly somewhere.

Play with it a bit more and realize the error isn't even stable.
The position error happened jumps randomly.

[jerch]

@mmis1000
For equality check of buffer content it might be better to do the JSON comparison on the private Bufferline objects, as they are the source of truth for that (it is ok to access privates in test cases). This has the benefit of revealing potential bugs in API mapping code as well.

[mmis1000]

@jerch
That probably won't work well currently, I guess?
Because I haven't handle the isWrapped yet (thus this is currently a draft)
And there is indeed some data in BufferLine that are not available to public api (the IExtendedAttrs).

BTW, it seems there is no way to copy a IBufferCell over another. But to handle the BCE, it seems I need to keep the cursor style as background style to decide whether add ECH or not. Should I just implement a fake class based on it?

[jerch]

Ah ok, yeah true. Sorry thought that is handled already.

The extended attrs is still field in progress for extended style, like different underline style and colors. It is still missing the frontend bits, thus its currently not officially supported yet. Backend PR #2751, more discussion about frontend needs in #1145).

Yes the private buffer cells are somewhat degenerate in their handling, they dont offer direct moving/copying around as this was not needed. They currently need the loadCell and write methods of a bufferline.

Hmm, well the private types actually form an inheritance chain for that purpose internally as AttributeData --> CellData. So far we had no need to expose AttributeData as well, maybe use a dummy IBufferCell and keep in mind to just look at its attribs and never its content (thats the only difference on the private types).

[mmis1000]

This should do a complete layout restore now (include line wrap).
The code probably still need some clear up for the left over during try and error.

About the performance, it handles about 5000 rows in 200ms on chrome.
(I am not sure whether it is fast enough. But given this is not designed to run on every tick, it is probably fast enough?)

[jerch]

@mmis1000 Imho speed is an issue for a serializer (as it has to be blocking to obtain a correct state). We already had quite the discussion about it when reshaping the buffer API (see #2369). The fastest serializer version I was able to find for basic serialization (doing only colors and flags) is at ~25MB/s for final string output (doing 100k scrollbuffer in roughly ~1s for single unoptimized run). Much higher than that is almost impossible with JS, as pure string concatenation power of JS gets the limiting factor. See #2369 (comment), there is also a branch where you can look at the code. But note that I used very different code than the final serializer.

Roughly estimated your current version should be somewhat around ~2MB/s, prolly 4MB/s for optimized runs.

[mmis1000]

I actually test for it a bit early today.

But the result it weird.

Profiler shows that most time was used on the _nextCell itself (the self is 81.3ms).
Even the complex check at line end use less time than it. (the 'self' is 1.3ms)
But the _nextCell is just a bunch of if else used to update current cursor style/position or _currentRow.

The only object it create is a array to hold sgr sequence which isn't different from the original code.
Do access this itself enough to slow down the v8?
Or due to string concatenation?

Edit: Do a test and realize that 30% time is caused by \x1b[${sgrSeq.join(';')}m
Move the \x1b[${sgrSeq.join(';')}m to _diffStyle shift 30ms run time to it.
It is actually a surprisingly slow operation

[jerch]

@mmis1000 Yes profiling JS is abit cumbersome, as all profilers I know do only sampling (snapshotting into callstack from time to time). Furthermore the JS engine might decide to inline hot functions, thus you would never see them being called, instead they add on top of the caller's time. The latter gets really annoying if you try to optimize hot paths, as they simply wont show up in the profiler records anymore.

Hand-waving rules I found to be working with V8:

• avoid object re-creation at any cost, instead resort to re-using previous ones (otherwise GC will kill perf)
• never change object shape by adding or removing properties (it will de-opt the whole function forever)
• deep nested branching is bad (similar as in C), this actually applies here, as a good JS engine will try to JIT hot functions
• Slicing logic into several sub functions is good, this gives the engine the freedom to decide, whether that particular logic is hot and should be inlined.

Inlining on its own is a tough topic and might be handled very different by various engines/optimizers. I only tested this with V8/crankshaft years back, but my guess is, that most ideas still apply to newer optimizers as well. For stable inlining it is important, that the arguments and the return values never change the types (e.g. if you expect a string as argument it is better to deal with an empty string instead of undefined, null, whatsoever). Ideally those functions are real functions in a functional programming sense (re-entrant), otherwise the optimizer might bail out later on for non obvious reasons. This can be achieved by a simple rule of thumb - calculate your function result only from arguments to avoid side effects while keeping the return type stable. Furthermore the function to be inlined should not be overly long in code size, otherwise the optimizer would not try to inline it at all (was about ~600 chars back then).

About string creation/concat:
Yes thats hard to do for JS. What looks nice and easy on code level, is a tough thing to achieve for the engine underneath in a speedy manner. Esp. there is a big difference between engines in loop-concat like for (c in arr) res += c; and array string joining as arr.join(''). The latter is much faster in V8, while loop-concat sucks big time. For Firefox it is the opposite where the loop is twice as fast compared to array join suggesting they have a special optimization for that.
I did not dig much further into this, as I mostly tested on V8, thus went with array join most of the time. Overall the array join is nearly the same on both, thus gives the most reliable results without penalizing one engine too much. It might be possible to speed this up further by collecting codepoints first and postponing the real string creation (idk, did not test that, also I'd see this as a premature optimization, as I would not expect much of a speed gain here).

[jerch]

@mmis1000 Is this in a review state yet or do you need some more bits/info to be addressed?

[mmis1000]

@jerch This is already in a review state. The part I mostly concern about is performance, but that part of code (create a array and join) is already existed in current code base. So I think it is better to left it for future pr instead of this.

[mmis1000]

@Tyriar
I removed the separate dummy buffer cell in favor of just remember where did the cursor style comes from.
And also, the test utility is moved into separate file. (Is it proper to put it in the test directory?)

I am not sure whether there is some edge case that I didn't handle correctly.
Probably it needs a fuzz test someday to check whether it will choke on some specified pattern.

[Tyriar]

@mmis1000 busy with VS Code endgame atm, I plan on getting to this next week.

[Tyriar]

Thanks @mmis1000, this works great. For the test change I moved it into page.evaluate, I mainly just wanted it inside the test/ folder

[cyrus-and]

Hi, sorry for bumping an old thread but, how does the cursor restoration work? I mean, it just works or do I have to enable it somehow? I don't see any difference in my use case (except for the background restoration part, which works as expected).

[cyrus-and]

Never mind, it was an issue on my side. Sorry for the noise.