Skip to content

Fix loadtest#156

Merged
Tyriar merged 1 commit intoxtermjs:masterfrom
Tyriar:fix_loadtest
Aug 31, 2021
Merged

Fix loadtest#156
Tyriar merged 1 commit intoxtermjs:masterfrom
Tyriar:fix_loadtest

Conversation

@Tyriar
Copy link
Copy Markdown
Member

@Tyriar Tyriar commented Aug 31, 2021

  • Wait for parsing finish before recording time
  • Use \r at end of each line so cursor doesn't move
  • Report MB/s

- Wait for parsing finish before recording time
- Use \r at end of each line so cursor doesn't move
- Report MB/s
@Tyriar Tyriar merged commit dc04723 into xtermjs:master Aug 31, 2021
@Tyriar Tyriar deleted the fix_loadtest branch August 31, 2021 19:49
@jerch
Copy link
Copy Markdown
Member

jerch commented Sep 1, 2021

@Tyriar Eww, the numbers are not very attractive (2-4 MB/s, but should be >>20 MB/s). Are you sure to measure the right thing? From the code it looks like the measuring contains the Math.random() construction as well, prolly the reason for the lowish numbers? For pure load/throughput on xterm.js I think it is better to construct the data upfront and only measure the time it takes from first to last term.write (last one with a callback with the time calculation).

@Tyriar
Copy link
Copy Markdown
Member Author

Tyriar commented Sep 1, 2021

Opps thought I was doing that already. I moved it down and it's similar though 🤔

image

@jerch
Copy link
Copy Markdown
Member

jerch commented Sep 1, 2021

Hmm thats weird, it misses at least a factor of 5. Were there recent perf regressions I am not aware of?

Edit: Hmm, is there something wrong with syncScrollArea? It accounts for half of the runtime here in devtools, haha.

@Tyriar
Copy link
Copy Markdown
Member Author

Tyriar commented Sep 1, 2021

syncScrollArea will take a decent amount of time until scrollback is filled, but after that it's still 5-10MB/s

@Tyriar
Copy link
Copy Markdown
Member Author

Tyriar commented Sep 1, 2021

Oh actually it is still showing up, with GCs happening as well, will investigate

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants