Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Render performance degradation from 3.0.2 to 3.7.0 #1677

Closed
tsl0922 opened this issue Sep 9, 2018 · 20 comments
Closed

Render performance degradation from 3.0.2 to 3.7.0 #1677

tsl0922 opened this issue Sep 9, 2018 · 20 comments
Assignees
Labels
area/performance type/bug Something is misbehaving
Milestone

Comments

@tsl0922
Copy link
Contributor

tsl0922 commented Sep 9, 2018

Update: This issue should be introduced between 3.3.0 and 3.4.0.


Originally reported at: tsl0922/ttyd#126

Details

  • Browser and browser version: Chrome69.0.3497.81
  • OS version: macOS High Sierra 10.13.6
  • xterm.js version: 3.7.0

Steps to reproduce

  1. Start the 3.7.0 demo.
  2. Open the demo with chrome, and turn on the FPS meter.
  3. Resize it to a larger size (155 cols / 40 rows).
  4. Run: vtop (npm install -g vtop).
  5. Press and hold the down arrow button.
  6. Result: 4-14 PFS on 3.7.0 (not stable) and 30-35 FPS on 3.3.0 (stable).

It's much smoother on 3.0.2 (ttyd 1.4.0) than 3.7.0 (ttyd 1.4.1).

@Tyriar
Copy link
Member

Tyriar commented Sep 9, 2018

I don't think this is anything to worry about, some history:

  • 3.0.0: Only cells that changed are re-rendered.
  • Introduced around 3.1.0(?), not yet fixed: the entire line gets re-rendered when a cell in the line changes. I have a WIP to fix this but I suspect this is what you're seeing. This makes rendering a lot slower (but still very fast).
  • Related: Introduced 3.5.0, fixed 3.7.0: the entire viewport was getting rendered almost every frame.

Let me know if that doesn't sound right for what you're seeing. A devtools perf profile would help better identify what's going on if not.

@tsl0922
Copy link
Contributor Author

tsl0922 commented Sep 10, 2018

@Tyriar This is still an issue to me, please keep it open, thanks.

I've updated the repro steps.

Introduced around 3.1.0(?), not yet fixed: the entire line gets re-rendered when a cell in the line changes. I have a WIP to fix this but I suspect this is what you're seeing. This makes rendering a lot slower (but still very fast).

Do you have any issue tracking this?

@Tyriar
Copy link
Member

Tyriar commented Sep 10, 2018

@tsl0922 I'm having trouble finding it. Can you provide a chrome perf timeline to show the difference you're seeing?

screen shot 2018-09-10 at 6 27 18 am

@Tyriar Tyriar reopened this Sep 10, 2018
@tsl0922
Copy link
Contributor Author

tsl0922 commented Sep 10, 2018

@Tyriar Uploaded profile data (tested with an old macbook air, the FPS is even worse):

profile.zip

@Tyriar
Copy link
Member

Tyriar commented Sep 10, 2018

@tsl0922 which OS, browser (including versions) were both of these profiles running on?

@tsl0922
Copy link
Contributor Author

tsl0922 commented Sep 11, 2018

@Tyriar macOS High Sierra 10.13.6/ Chrome 69.0.3497.81

@Tyriar
Copy link
Member

Tyriar commented Sep 11, 2018

@tsl0922 and this is xterm 3.0.2 vs 3.7.0? Can you also do the same against 3.4? 3.4 should have the line fix but not the dynamic cache parts.

@tsl0922
Copy link
Contributor Author

tsl0922 commented Sep 11, 2018

@Tyriar yes its xterm 3.0.2 vs 3.7.0.

3.4.0 still have this issue as 3.7.0, but 3.3.0 works good, so I've downgraded xterm to 3.3.0 in ttyd now and the reporter confirmed this fixes the issue(tsl0922/ttyd#126 (comment)).

profile-330-340.zip

@Tyriar
Copy link
Member

Tyriar commented Sep 12, 2018

Hm, well 3.4.0 is when the dynamic cache was introduced which touched a lot of stuff #1327

@Tyriar
Copy link
Member

Tyriar commented Sep 12, 2018

@tsl0922 yeah I think you're on to something, on my macbook I'm seeing approx the following numbers:

  • 3.3.0: ~1.5ms frames
  • 3.4.0: ~3ms frames (static cache)
  • 3.7.0: ~7ms frames (dynamic cache)

After a look at the code it may just be the additional of the abstractions/functions/GC 🤔. I certainly don't expect the numbers to have degraded that much.

@Tyriar Tyriar added type/bug Something is misbehaving area/performance and removed needs more info labels Sep 12, 2018
@jerch
Copy link
Member

jerch commented Sep 12, 2018

The profile shows like >90% of the runtime is spend in _drawUncachedChars. Isnt this the static cache fallback?

@tsl0922 Does your input contain lots of different chars that are not in the ASCII range? Tbh both profiles look not so good regarding drawing to total runtime ratio. Could you check chrome://gpu if the browser is using hardware acceleration at all?

@tsl0922
Copy link
Contributor Author

tsl0922 commented Sep 12, 2018

@jerch chrome://gpu shows: Canvas: Hardware accelerated.

This issue can be reproduced with vim too: just open source file with syntax highlighting and press j to scroll.

@Tyriar
Copy link
Member

Tyriar commented Sep 12, 2018

@jerch did you switch to dynamic cache? I'm trying to narrow down what's going on and I'm eyeing these lines atm:

String generated every char draw:

return `${glyph.bg}_${glyph.fg}_${styleFlags}${glyph.chars}`;

Object created every char draw:

{chars, code, bg, fg, bold: bold && terminal.options.enableBold, dim, italic},

@Tyriar
Copy link
Member

Tyriar commented Sep 12, 2018

I'm also seeing the static atlas on the master branch drawing rows around 30% faster which I would largely put up to the string creation.

@jerch
Copy link
Member

jerch commented Sep 12, 2018

@Tyriar
Ah yes, few weeks ago during some benchmarking regarding the typed array buffer I figured those interim objects are very expensive. Most of the time I test in the demo which happened to default to the static cache, so I am not very deep into benchmark numbers of the dynamic version.

@jerch
Copy link
Member

jerch commented Sep 12, 2018

@Tyriar Could the cache key string be replaced by some integer? A template string creation vs. an integer on every char should make some difference.

@Tyriar
Copy link
Member

Tyriar commented Sep 12, 2018

@jerch yeah I think could probably pack an integer with all that data (ascii+256 color).

I still don't get why the drawImage calls seem to cost so much more in the later version 😕

screen shot 2018-09-12 at 12 17 18 am

Perhaps because it's drawing from a canvas now and not a bitmap?

@Tyriar
Copy link
Member

Tyriar commented Sep 12, 2018

Compare that to 3.3.0, note that all characters in the row appear to be getting drawn still:

screen shot 2018-09-12 at 12 28 12 am

@Tyriar
Copy link
Member

Tyriar commented Sep 12, 2018

I think I fixed it, I removed a bunch of the object/string overhead and drawing from an ImageBitmap seems to make the dynamic cache go down to around 3ms on average (from 7). PR probably tomorrow.

@jerch
Copy link
Member

jerch commented Sep 12, 2018

Wow nice 👍

Also chrome got much much better in drawing things to canvas since version >65, the difference here between electron v2 and the newer browser is really impressive (like 3-5x faster in my tests). Maybe its broken again with v69 (not yet tested, still v68 here)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance type/bug Something is misbehaving
Projects
None yet
Development

No branches or pull requests

3 participants