Xterm.js: 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:

https://github.com/xtermjs/xterm.js/blob/0e45909c7e79c83452493d2cd46d99c0a0bb585f/src/ui/RenderDebouncer.ts#L28-L29

https://github.com/xtermjs/xterm.js/blob/0e45909c7e79c83452493d2cd46d99c0a0bb585f/src/ui/RenderDebouncer.ts#L47-L48

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 https://github.com/xtermjs/xterm.js/pull/1622

This commit https://github.com/xtermjs/xterm.js/commit/34eb08952c8e78b20ea7109403f244d82c63331c 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 :smiley: