move _data from BufferLine to new LogicalLine class

[PerBothner]

This moves cell data from a BufferLine into a new LogicalLine class which contains the actual data for a line. This data does not change if Buffer width changes. Each LogicalLine is rendered as one or more BufferLines, depending on terminal width; conversely each BufferLine specifies a sub-range of the parent LogicalLine.

This implements issue #5673 .

Some benefits:

• Simpler and faster Buffer.resize.
  No need for cleaning up memory after resize.
• Potentially handle reflow lazily, only reflowing visible lines.
• Faster getBlankLine.
• Do not need memory for empty cells at end of lines.
• Better conceptual model/view separation: The LogicalLine is the "model" (conceptual buffer data) while the set of BufferLines is the "view" (how the LogicalView is rendered given a specifical line width).
• Potentially support multiple "views" of the same model at the same time. For example people might be collaboratively viewing the same terminal, but with different line widths.
• End-of-line background color persists over resize.

Memory cleanup: There is no need for queing up memory-cleanup items when doing a resize, since resize doesn't create garbage (except when lines are trimmed). However, it may be worthwhile doing memory cleanup when lines are erased and/or when lines are unwrapped (commonly done together). We might also cleanup memory when lines move out above ybase.

Merging BufferReflow.ts into Buffer.ts: An optional follow-up would be to merge the rest of BufferReflow.ts into Buffer.ts - since there isn't much left in BufferReflow.ts. See this changeset. That would make more slightly more efficient and (in my opinion) more readable code, but I didn't do this in the interest of a more manageable PR.

[PerBothner]

Note this PR assumes PR #5788 has been merged first.

[PerBothner]

FWIW, I'm looking into adding addons-search support to the DomTerm port, and it appears using LogicalLine can somewhat simplify the implementation of translateBufferLineToStringWithWrap.

(A possible future improvement I'm thinking about is to store the actual values of translateToString in the LogicalLine, replacing the _combined map. This would eliminate the need for the SearchLineMap.)

[PerBothner]

I merged from upstream/master (with some effort thanks to the new string aching). However, the image addon breaks the BuferLine abstraction. I'll take a look at that shortly. I may focus on fixing that in the context PR #5853, which I haven't merged yet. (I would also like to further simplify the reflow logic, which I hope allow me fix the remaining testsuite failures.)

[PerBothner]

The integration failures are because addon-image accesses BufferLine internals. PR #5879 is how I suggest fixing that problem. Otherwise, I think this ready for review and (if #5879 is merged) merging.

Compared to PR #5853, this PR is quite conservative. It does not make changes to Markers. For buffer resize/reflow the keeps the existing algorithm and much of the same code, but it should be substantially faster because it does not have to copy cells. (PR #5853 makes reflow even faster and simpler.) If preferred, I can create a PR that is "in-betwen" this one and PR #5853 - we can attach Markers to LogicalLines, but leave Decoration handling unchanged.

Note the string-cache works on BufferLine. A plausible follow-up change would be to cache strings for each LogicalLine instead: If you need the string value of a line, you're more likely to need it for a LogicalLine rather than a BufferLine. Examples include searching and URL matching.

[PerBothner]

I will be happy to update this PR the latest master - as soon as I get some feedback about this proposal, and some indications that my contributions are at least considered. As long as I am being totally ignored (see issue #6022) trying to keep this PR updated is a waste of my time.

[jerch]

I just perf tested this PR with the quickfixes from #6106 (applied where applicable):

• Chrome: ~5x slower (processing went from 1900 ms to ~10 s)
• Firefox: ~5x slower (processing went from 4000 ms to ~20 s)
• lots of unfinished frames in profiling data

Is this yet optimized by any means? Because 5x slower is a clear no-go.

[PerBothner]

Hm. That is disappointing. I have not done any measurement or performance tuning. I can't think of a reason for the slowdown - I'm hoping it is some specific hotspot or screw-up that is causing the regression. I may not have time to investigate today, but hopefully tomorrow. Hoping reflow at least is significant faster (and should be even faster with followup changes).

[PerBothner]

A quick test finds that LogicalLine._resizeData is called way too much from setCellFromCodepoint. It needs to be smarter.

[PerBothner]

I checked in a tweak to _resizeData that should improve things. Still a work-in-progress.

[jerch]

Yupp this changed the picture massively:

• Chrome: 1900 ms vs. 4800 ms
• Firefox: 4000 ms vs. 5000 ms

_resizeData is still quite high in the books - could we somehow tweak it to run only once per print invocation? (Background: a print call should not be longer than a logical line, but might be shorter)

On a sidenote - what happens, if a console app spits out a 10GB line without any NL in between?

[PerBothner]

_resizeData is still quite high in the books - could we somehow tweak it to run only once per print invocation?

I though of something similar. It is conceptually easy to bundle a run of "simple" code-points to a single call to something like a setCellsFromCodepoints (not plurals), assuming all characters in run have the same width and there are no combining characters. Of course, the logic in print is somewhat complicated, and this would make it worse.

An alternative could be to pre-process all the codepoints, storing the the width and combining data into a helper Uint8Array. Then we know the actual width in columns and hence the number of cells needed. However, that is a bigger change than I would like to make right now, partly because changes like these might change how we go about making such a change.

I'm inclinded to use a heurtistic (guess). Before the main call to setCellFromCodepoint (at the write current char to buffer and advance cursor comment), we add a "hint" call to _rewriteData. Maybe:

if (pos === start) bufferRow._resizeData(col + chWidth * (end-start));

This works exactly if all the characters in the call to print have the same width and none are combining. If that is not the case, it is an over-estimate if there are combining characters, or the first character is double-width and most of the rest are not; it is an under-estimate if first character is single-width and not all of the rest are. Probably good enought for now. Especially since _resizData by itself adds a bit of "slop".

[PerBothner]

Is there some writeup on recommended ways to test for performance? Perhaps part of or linked to from CONTRIBUTING.md?

[jerch]

For the parser side of input chain you can use this to get a rough number:

npm run benchmark out-test/benchmark/Terminal.benchmark.js

It skips the renderer path, so is just WriteBuffer, parser an terminal buffer handling.

If the number are totally off, you have to use your browser with devtools profiler. Current issue there is, that the setTimout/clearTimeout cascades skew the numbers heavily. To get halfway reliable numbers in devtools you'd have to apply half of the quickfixes listed in #6106 (at least the TimeoutTimer fixes). It is annoying, that we are in this situation, but it is what it is.

[PerBothner]

Possible optimization/trick for scroll: Allocate a fresh _data array that is the exact needed size for the old (scrolled-up) line, and copy the old contents from the old line. Then re-use the old buffer for the new scrolled-in line. (If the old buffer grew to some really huge size, like multiple kBs, optionally replace it with something more normal, like 500*3.) Basically, we use a "work" buffer while actually updating the line.

Then effectively you only get one allocation for each line, and that allocation is the optimal size. However, each line requires extra copy at the end, but presumably that should be a highly efficient operation.

The tradeoffs and the logic becomes more complicated if old lines are frequently updated. Not sure the best strategy to handle that. Possible use different strategies for normal buffer and alternate buffer.

[jerch]

Possible optimization/trick for scroll: Allocate a fresh _data array that is the exact needed size for the old (scrolled-up) line, and copy the old contents from the old line. Then re-use the old buffer for the new scrolled-in line. (If the old buffer grew to some really huge size, like multiple kBs, optionally replace it with something more normal, like 500*3.) Basically, we use a "work" buffer while actually updating the line.

That sounds very much like what recycle + copyFrom does - it takes the memory of an old bufferline to clear it with a single .set call to empty it. Whether .set empties or pulls over data, is the same speed-wise. But the additional attributes are much more heavy to pull over (the reason why copyFrom slowed down that much, although it does not need to look at additional attributes).

But I still wonder - how does LogicalLine deal with overlong lines in MBs or GBs? You would need a sanity upper limit, where lines get artificially split again to not cause OOMs. Wouldn't that negate the whole effort of LogicalLine?

[PerBothner]

But I still wonder - how does LogicalLine deal with overlong lines in MBs or GBs? You would need a sanity upper limit, where lines get artificially split again to not cause OOMs. Wouldn't that negate the whole effort of LogicalLine

I believe the scrollback limit will "trim" excess visible (non-logical) lines. So if a line is extremely long it will be wrapped in print, entered in the lines table - and the excess trimmed. (This assumes you have a finite non-huge scrollback limit.) This concern may preclude some optimizations where we defer line-breaking a the newline is seen (or rendering); the current PR does not do that.

I'm not sure that using LogicalLine makes much difference in terms of memory - roughly the same amount is used either way. A really long line will require one long _data array rather than many smaller ones, so allocation may fail a bit earlier. But if you're using that much memory, I suspect you're screwed regardless.

At a higher level: Are we really that concerned if the the terminal runs out of memory on being fed GBs of garbage?

[PerBothner]

Is there a way to run the benchmark inside a non-headless terminal? That might be helpful to find hotspots and otherwise debug issues.

[jerch]

Is there a way to run the benchmark inside a non-headless terminal? That might be helpful to find hotspots and otherwise debug issues.

No that is sadly not possible anymore (a previous version of xterm-benchmark had chrome-timeline package for a browser based runner, but that package is outdated, as the chrome changed the devtools interfaces). The repro steps in #6106 are basically what that benchmark does. the OSC commands are needed to spot the end of data condition. This is needed as long as the code still contains the toxic setTimeout chains, and for closing devtools in between to see the real throughput (the devtool profiler sucks hard on IO heavy benchmarks always screwing up the numbers).

This concern may preclude some optimizations where we defer line-breaking a the newline is seen (or rendering); the current PR does not do that.
...
At a higher level: Are we really that concerned if the the terminal runs out of memory on being fed GBs of garbage?

Yes that is where the concern comes from. The ring buffer logic cannot run into that hazard, as it always introduces cuts in memory at terminal width. But since LogicalLine tries to merge things I was just wondering but degraded data. And this can happen pretty easily by catting /dev/urandom or any endless producer into a terminal. Not very helpful to anyone, but a terminal may not crash from that either.

[PerBothner]

Fully updated and tests pass.

There is still a significant slowdown running Terminal.benchmark.ts though not as bad. Not quite sure why. I've done various optimizations, including creating a new function setCellsFromCodepoints to bulk insert from print. It is possible adding extra LogicalLine objects, plus extra indirections, might add up in small ways. Testig "Performance" in the browser debugger doesn't show me any obvious culprits. Suggestions welcome.

That said, I believe this re-factoring will enable some other optimizations:

• The _chars proposal will allow getting rid of the _combinedData object and field, which saves at least an object allocaton per line. It also gets rid of the separate string cache.

• We can greatly simplify and speedup resize/reflow. This current PR is "optimized" for easier review, and so the changes are modest. For a more thorough re-write, see the reflowRegion function in this. Perhaps it would be preferable to merge in those changes into this PR - let me know.

• We can attach a list of Marker objects to each LogicalLine. This should be simpler and faster, especially if we can remove the need for the line field in each Marker. That in turn may enable improvements/replacement of ExtendedAttrs and/or Decorations.

• Search and other places that call translateToString would be cleaner and more efficient if called on LogicalLine. This especially makes sense in conjunction with the _chars proposal.

Let me know if you prefer me to complete to resize/reflow optimization before or after review of this PR. I can also implement the _chars idea.

[PerBothner]

This has been superceded by the _chars PR #6160, which is based on this LogicalLine change. To avoid confusion, I intend to close this PR and suggest that future discussion be in PR #6160.