Xterm.js: Refresh limit system makes performance bad on Windows

[Tyriar]

The integrated terminal in VS Code has a performance issue on Windows outlined here https://github.com/Microsoft/vscode/issues/10328#issuecomment-248116656, @codec-abc suggests it's due to the limit system in the refresh function:

setTimeout(function () {
  self.refresh(start, end, false);
}, 34)

Details

• Browser and browser version: vscode v1.6/electron
• OS version: Windows 10
• xterm.js version: https://github.com/sourcelair/xterm.js/commit/3c9d8b0d84e9262c013a7698aad57f02cccad2f3

[codec-abc]

This may be not totally related but do you think the refresh frequency could be dynamic ? Because I think it would be better for the user experience to have a default 60 fps refresh rate. The terminal will probably feel more responsive.

[jerch]

@codec-abc 60fps is unrealistic for high output programs. High output means many changes of the DOM which increases the penalty for the reflow. Sticking to 60fps would therefore slow down the output flow. It might be a goal though if the stream parsing and state handling would have their own thread.

@Tyriar I suggest to bind the refresh to an animation frame to avoid cancelled reflows:
requestAnimationFrame(function() {self.refresh(start, end, false);});
(maybe in combination with setTimeout from above, if throttling is still needed).

[Tyriar]

Far too long is spent in Viewport.syncScrollArea:

Also strangely Viewport.refresh is being triggered multiple times during a single syncScrollArea, not sure why.

[jerch]

@Tyriar I think this due to the fact that syncScrollArea is bound to the scroll-event of the terminal, which is likely to be triggered several times in term.write.
The bad runtime is mostly caused by forced reflows by accessing getBoundingClientRect and this.viewportElement.scrollTop in syncScrollArea.

I append a small patch which reduces getBoundingClientRect calls and avoids scrollTop read access completely. Not sure if it generates still the same output, playing around with charMeasureElement showed weird changes in the output (missing lines and such).

diff --git a/src/Viewport.js b/src/Viewport.js
index d23ba7c..df87799 100644
--- a/src/Viewport.js
+++ b/src/Viewport.js
@@ -16,6 +16,8 @@ function Viewport(terminal, viewportElement, scrollArea, charMeasureElement) {
   this.viewportElement = viewportElement;
   this.scrollArea = scrollArea;
   this.charMeasureElement = charMeasureElement;
+  this.charSize = this.charMeasureElement.getBoundingClientRect();
+  this.scrollPending = false;
   this.currentRowHeight = 0;
   this.lastRecordedBufferLength = 0;
   this.lastRecordedViewportHeight = 0;
@@ -34,7 +36,7 @@ function Viewport(terminal, viewportElement, scrollArea, charMeasureElement) {
  *   doesn't exist it will be created.
  */
 Viewport.prototype.refresh = function(charSize) {
-  var size = charSize || this.charMeasureElement.getBoundingClientRect();
+  var size = charSize || this.charSize;
   if (size.height > 0) {
     var rowHeightChanged = size.height !== this.currentRowHeight;
     if (rowHeightChanged) {
@@ -78,9 +80,15 @@ Viewport.prototype.syncScrollArea = function() {
   }

   // Sync scrollTop
-  var scrollTop = this.terminal.ydisp * this.currentRowHeight;
-  if (this.viewportElement.scrollTop !== scrollTop) {
-    this.viewportElement.scrollTop = scrollTop;
+  var self = this;
+  if (!this.scrollPending) {
+    this.scrollPending = true;
+    requestAnimationFrame(function(){
+      // hack - simply scroll to bottom assuming container
+      // is never higher than the hardcoded value
+      self.viewportElement.scrollTop = 3000000000;
+      self.scrollPending = false;
+    });
   }
 };

Just tested it in VSCode 1.5.3 under Windows - it will not work there without a few changes (for some reasons the metrics of this.charMeasureElement is zeroed at the beginning while it isn't with the xterm.js demo. Also it seems more like a hack than a real solution ;)

[Tyriar]

This is very obvious when you compare xterm.js within vscode to hyper, it helps that their scroll bar seems to be only for show though (no need to deal with scroll events).

[parisk]

Stating the obvious: I think we should tear down the whole rendering part and reduce as much waste as possible for start.

I am not sure if I can get on top of this right now, but I can definitely help if you are willing to take this over @Tyriar.

[Tyriar]

I'd still like to do it when I get a chance, the CircularList scrollback stuff seems like a good starting point for the performance issues.