xterm.js
xterm.js copied to clipboard
xtermjs
some buffer handling optimizations
Should fix #4112.
Still WIP...
TODO:
- [x] alloc/copy-free shrinking
- [x] alloc/copy-free enlarging within bytelength
- [ ] postpone memory resize if buffer > 2x view size
- [ ] browser: use
requestIdleCallback - [ ] nodejs: shim somehow
- [ ] browser: use
- [ ] optimize
copyCellsFrom, check for correct _combined and _extendedAttr handling
shim somehow
Alternatively we can just not do this optimization for xterm-headless, adding it later on if/when requestIdleCallback is added.
About lazy resizing of the underlying array buffer - see https://github.com/xtermjs/xterm.js/pull/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.
@jerch :+1:
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 ☹️
@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...)
@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.
@Tyriar Ready for review.
Note that I changed performance.now to Date.now, which also fixes IdleTaskQueue for nodejs.
@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 ...
If you think it's worth it, the PR is way better than the current state already 🙂
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). :sweat_smile:
@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 :rofl:
If you want to check again, imho this is ready to get added. :sweat_smile: