Performance regression with maximized window on 2.0.0-canary.9

[chabou]

• I am on the latest Hyper.app version
• I have searched the issues of this repo and believe that this is not a duplicate

• OS version and name: macOS Sierra (10.12.6 (16G29))
• Hyper.app version: 2.0.0-canary.9

Issue

We have a huge perf regression with xterm3 with a big/maximized window (using vi for example)

xterm2 (2.0.0-canary.8)

xterm3 (2.0.0-canary.9)

Performance recording with xterm3

Try with last 1.8.2-beta.3 Electron version and no change.

I have same performance issue with VSCode

cc @Tyriar

[Tyriar]

I'm guessing this is what @rauchg was seeing. I suspect this branch will fix the issue https://github.com/Tyriar/xterm.js/tree/1063_minimize_drawing by no longer redrawing everything but only the first 4 columns.

[Tyriar]

Upstream issue: xtermjs/xterm.js#1063

[Tyriar]

@chabou also how big is your windows cols/rows exactly when you're seeing these 500ms frames?

[ppot]

Looking at this it look like it's close to be fullscreen?

[bgw]

I'm running into this issue too. It's especially bad on my 1440p screen, to the point where I can't use hyper, since every keystroke in vim causes a 300ms (or more) frame.

I tend to have around 320 columns and maybe 90 rows or so.

[Tyriar]

I cannot reproduce close this this level of slowdown.

Can you share your vi config? Something that could potentially explain this is if you're styling the text/backgrounds using truecolor/255 color?

[bgw]

Here's my vimrc. It's not easy for anyone to pick up and use though, because it depends on a large set of plugins installed using git submodules.

It turns out that I was setting the background color with vim (though I was just setting it to the same color as my terminal's background, and I'm not using 256 colors). Running :hi Normal gives:

Normal         xxx ctermfg=7 ctermbg=0 guifg=#d8d8d8 guibg=#181818

and running :hi Normal ctermbg=none improves performance significantly.

A took a look at how alacritty does this, and it looks like they populate a glyph cache/atlas when drawing characters, so that the cache is filled with character color combinations that are actually in use, whereas xterm.js is just using a static cache. I'd be interested in trying to implement a similar approach (maybe a simple LRU cache) in xterm.js.

[passcod]

Unsure if this is related, but I have a 4k screen, Hyper maximised, and experiencing >1s pauses. Worse in vim, going up sometimes to ~10s pauses. Doesn't happen on stable.

[Tyriar]

@bgw yeah we only cache with the standard background, and then turn the bg transparent to retain SPAA and make it look decent with overlapping chars.

A dynamic cache is something I've been thinking about for a while, it would definitely be cool but it would be a fair bit of effort to implement.

[chabou]

I'm using https://github.com/yosiat/oceanic-next-vim Color theme.

Testing with a 320colx90rows term : frame render time is 500ms in vim.
I can confirm that setting ctermbg=none on Normal preset reduces frame render time to 80ms.
Disabling this theme (it uses some other ctermbg) reduces it to 50ms.

Very nice catch @bgw 👍

[Tyriar]

There are also huge gains that could be made if the user doesn't care about subpixel AA as then the background color doesn't need to be cached as well.

[rauchg]

I can also confirm that disabling the theme made it mindbogglingly fast 🤣

[rauchg]

Great detective work @bgw !

[Tyriar]

On second thought we would still need to check the background, but it would be based on the luminance of the color, not the exact color. And some backgrounds should be able to share without it looking weird.

[portenez]

+1 on this issue, I encountered it this morning as well. canary9 caused this, but not canary 8.

[portenez]

Disabling the theme made it fax again. I was using oceanic next theme as well. I'm using nvim, and couldn't quite apply @bgw's fix. I guess I just don't know how to apply it.

[hew]

I found that disabling

• colorscheme
• set termguicolors

Gives me the best workaround performance on Neovim.

[portenez]

I removed set termguicolors and the problem seems to be gone. performance is decent in neovim after that change.

[bgw]

FYI, I'm working on an implementation of a dynamic character atlas for xterm.js that allows characters with different background colors to be cached, which should fix most people's performance issues. It generates a character atlas that looks something like:

The current implementation is here, but it's still pretty buggy, and needs a bit of work and cleanup before I'm comfortable making a PR out of it, but things are coming along!

cc @Tyriar

[Tyriar]

@bgw nice! I had a quick look at your branch, this was the main bit I'm worried about:

"lib": ["DOM", "ES6", "DOM.Iterable", "ScriptHost"]

I think using these feature would break some browser support like IE11.

[Tyriar]

@bgw I created xtermjs/xterm.js#1294 to track this, the issue I pointed to before xtermjs/xterm.js#1063 is a different set of optimizations that can be tackled separately.

[albinekb]

Why do you need IE 11 support @Tyriar Isn't performance more important than compatibility for xterm? 😄 Is there anyone using xterm in IE11? It was outdated 4 years ago

[Tyriar]

@albinekb well it's more about developer convenience than performance, but IE11 support is needed to run xterm.js within Visual Studio 🙂 https://github.com/Microsoft/WhackWhackTerminal

[Tyriar]

@bgw also are these different characters? Looks like 3 space chars?

I notice 3 dark grey ones too?

[bgw]

@Tyriar I need an ordered map implementation for the LRU cache, so I'm relying on the ES6 Map implementation, which guarantees insertion order iteration. We'll either need to pull in a polyfill, or I'll need to write a minimal ordered map implementation myself. Alternatively, we can try relying on the iteration order of objects in JS, but that's also a fairly new addition to the spec, though every major browser does implement it.

Regarding the space characters: I use powerline, and that screenshot is from the demo page which doesn't use the right font, so I'm guessing it's just a couple of characters that didn't render quite right.

The diff isn't ready for review yet, I just wanted to let people know that I'm working on it, so that nobody else starts to work on the same thing.

[Tyriar]

@bgw

We'll either need to pull in a polyfill, or I'll need to write a minimal ordered map implementation myself.

Probably a polyfill is the way to go, we can tackle that later though.

Regarding the space characters: I use powerline, and that screenshot is from the demo page which doesn't use the right font, so I'm guessing it's just a couple of characters that didn't render quite right.

Makes sense.

---

Something else you might notice with the current implementation is that the default background color is pulled off of characters in the character atlas, this is so characters that are overlaid on wide glyphs will not completely override them. An example:

We may just want to draw uncached for character that are on top of wide chars to avoid the bad antialiasing. As you can see wide characters are also something else to consider, let's try target just plan single width characters with this change initially before trying to cache wide ones 😄

[bgw]

FYI: Even with the dynamic char atlas patch set, drawing characters with a background is slower than without, since we make a new call to fillRect for each character (even when adjacent characters are the same color), which ends up being expensive (it takes up nearly half the render time for me).

I'm hoping to tackle that problem next, but you might still want to stick with the :hi Normal ctermbg=none hack for now.

EDIT: Oh, and let me know if you encounter any issues with the new atlas!

[chabou]

@bgw Thank you so much for your hard work. This is totally amazing. Be sure, that we'll give you our feedback.

[bgw]

FYI, xtermjs/xterm.js#1413 should negate the need for the :hi Normal ctermbg=none hack once it lands (and hyper cuts a release including it), since it makes the background drawing operation an order of magnitude faster.