some buffer handling optimizations

[jerch]

Should fix #4112.

TODO:

• alloc/copy-free shrinking
• alloc/copy-free enlarging within bytelength
• use DebouncedIdleTask to defer array buffer resize after shrinking
• optimize copyCellsFrom, check for correct _combined and _extendedAttr handling not critical at all
• several unit tests

[Tyriar]

shim somehow

Alternatively we can just not do this optimization for xterm-headless, adding it later on if/when requestIdleCallback is added.

[jerch]

About lazy resizing of the underlying array buffer - see #4144

@Tyriar I currently dont like the idea to place lines * DebouncedIdleTask objects on the terminal buffer, also placing a cleanup task for every single line might grow really big (easily going into several MBs for scrollback >1000 just for registering the cleanup work). Instead I will try to find a way to batch the cleanup from Buffer or CircularList side (maybe in 1k steps), which only needs 1 DebouncedIdleTask object. This might be a rare use case for a WeakSet, will see if that comes handy to hold refs to lines, that need a cleanup.

[Tyriar]

@jerch 👍

This might be a rare use case for a WeakSet, will see if that comes handy to hold refs to lines, that need a cleanup.

I always think something will work for WeakSet and it doesn't end up working out. I tried recently but the fact that the keys aren't enumerable and they cannot be numbers meant It wasn't appropriate ☹️

[jerch]

@Tyriar Yeah WeakMap and WeakSet are very awkward and mostly not of much use. Will see if its useful here (have currently benchmark issues, seems master has a bad perf issue, my typical ls -lR /usr benchmark takes like 4 times longer since I merged master back here, still investigating...)

[jerch]

@Tyriar Plz have a look at the way I use DebouncedIdleTask on Buffer. Resizing is quite fast that way (~120ms), furthermore it splits its memory alloc workload into smaller batches, that dont interfere too much with mainthread latency (its at ~13ms on my machine for 5k lines, can do smaller batches if needed). Also the batching should be resilient to in-between resizes altering bufferlines again before the memory got fully fixed (well, still needs some unit tests).

NB: I think there is still a catch in DebouncedIdleTask, as I get Uncaught ReferenceError: performance is not defined on nodejs. Is that unit tested to work on nodejs?

[Tyriar]

Sorry about the delay, didn't realize this one was waiting on me 😅

[jerch]

@Tyriar Saw that you use performance.now() in several places in TaskQueue.ts. I suggest to replace it with Date.now(), as it is typically 4-6 times faster than performance.now() [*]. Imho both give enough time resolution for the tracked tasks, only downside of Date.now() its dependence on the wall clock, thus it would shift on machine clock changes (can be mitigated by some lower bounds, like always accounting at least 1msec).

---

[*] Observed the bad runtime of performance.now in my fifo pipe buffer impl in browser-fakepty, where performance.now throttled pipe throughput to ~200 MB/s, while it happily runs at ~1GB/s with Date.now.

[jerch]

@Tyriar Ready for review.

Note that I changed performance.now to Date.now, which also fixes IdleTaskQueue for nodejs.

[Tyriar]

Tested and lgtm!

[jerch]

@Tyriar Oh well - should we still change the rescan logic as described here?

I think a slightly more advanced logic might avoid all those re-scan refforts:

• safe last adjusted line number
• continue from last adjusted one
• when done, recheck all again to spot in between buffer shifts
• do this until exhausted

This would work pretty much the same as now, but not re-scan previous lines everytime. Instead it only does a final scan, before it is really finished. With that imho lower lines handled at once should not show that exp growing anymore.

Would be a more "heuristical" approach avoiding the bad exp rescan runtime for n scrollback lines ...

[Tyriar]

If you think it's worth it, the PR is way better than the current state already 🙂

[jerch]

Holy cow, just tested it and what can I say - never underestimate the effect of exp growing, even for cheapo tasks like rescanning arrays in JS. Here are some numbers for 100k scrollback compared to before, with 100 (!) lines batched:

• before: 3400 ms spread over ~4s
• with new approach: 140 ms, spread over 270 ms

Outch, I'd say the result tells - nope, try to avoid exp runtime at any costs (well beside exp costs lol). 😅

[jerch]

@Tyriar Thx for insisting on the lower batch size in the first place, helped me to think around the nonsense re-scan penalty with exp growth - this runtime is better invested on coin miners 🤣

If you want to check again, imho this is ready to get added. 😅

[Tyriar]

Nice 👍