Full viewport refreshing more often than necessary

[princjef]

Type	Version
OS	Windows 10
Browser	Electron 2.0.7
Xterm	3.6.0

---

From some profiling/investigating I've been doing, the renderer appears to be doing more work than needed when simple inputs are provided (such as individual characters typed at the input prompt).

To reproduce this, I ran the following:

1. Ran the xterm-electron-sample project
2. Made the window full screen (1080p screen in my case) and ensured every line had something printed on it by running ls until the screen was full.
3. Opened the dev tools (Ctrl+Shift+I) and started a profiling session
4. Started typing random ascii characters on the shell prompt and recorded the render times for each keystroke.

Under these conditions with xterm 3.6.0, I'm seeing an average of 45-50ms to render each frame, even though there is only one line that is actually changing with each keystroke. Having looked through the code, the render call is explicitly told which line range to render, and yet still seems to be re-rendering everything each time.

I did some more digging and I think the problem comes from a combination of these two sections:

xterm.js/src/ui/RenderDebouncer.ts

Lines 28 to 29 in 0e45909

	this._rowStart = this._rowStart !== undefined ? Math.min(this._rowStart, rowStart) : rowStart;
	this._rowEnd = this._rowEnd !== undefined ? Math.max(this._rowEnd, rowEnd) : rowEnd;

xterm.js/src/ui/RenderDebouncer.ts

Lines 47 to 48 in 0e45909

	this._rowStart = null;
	this._rowEnd = null;

The first is doing an explicit check against undefined to handle the case when there are no bounds already set, but the second resets the values back to null after each render cycle, thereby bypassing the check. (point of interest: Math.min(null, 10) === 0)

I was able to dramatically improve the render time by changing the assignments in the second block to assign undefined instead of null. With the same test case, I'm now seeing rendering times of 4-5ms, instead of 45-50ms.

I'm happy to shoot over a PR to change it but wanted to double check the intent first. It looks like the checks were changed from checking against null to checking against undefined intentionally so I wasn't sure if I'm just missing something.

[Tyriar]

@princjef ugh, this is terrible. I have been getting much more performance reports and it looks like this was the problem. I put together a PR as I wanted to fix this ASAP due to the impact #1622

This commit 34eb089 was meant to check for the 0 case only, but I didn't realize null was being set ☹️

[princjef]

@Tyriar no worries, just glad it helped 😃