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

Use microtask to flush the write buffer after input #4159

Merged
merged 2 commits into from
Sep 28, 2022
Merged

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Sep 28, 2022

Fixes #4158

Previously this was 5000ms+ scripting:

Screen Shot 2022-09-28 at 9 05 58 am

After input:

Screen Shot 2022-09-28 at 9 07 45 am

@Tyriar Tyriar added this to the 5.1.0 milestone Sep 28, 2022
@Tyriar Tyriar requested a review from jerch September 28, 2022 16:08
@Tyriar Tyriar self-assigned this Sep 28, 2022
// otherwise schedule for the next event
if (this._didUserInput) {
this._didUserInput = false;
queueMicrotask(() => this._innerWrite());
Copy link
Member

@jerch jerch Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If latency is your main concern here - I think we can call this._innerWrite() directly here? (after pulling the last 3 lines of the method before the call) I dont see a reason for microtask delegation here, but there is a chance I am overlooking something.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you're right, this made more sense in the previous iteration when I was relying on data length instead of the user input signal, will change

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to duplicate some calls to make this happen, that's one of the reasons why it was microtask

@jerch
Copy link
Member

jerch commented Sep 28, 2022

@Tyriar Yepp, the direct call works nice for me.

Also this PR restores scripting time back to old values, my devtools profiling is still screwed up:
grafik

Note this huge greyish portion and the overall runtime. Any idea what causes this? Also note that with devtools closed I see normal runtimes. My guess would be - there is another code change, that s*cks really bad on profiler performance, but gets not really resolved by the profiler (thus shown as grey). Sadly this currently makes the devtools profiler unusable for me.

@Tyriar
Copy link
Member Author

Tyriar commented Sep 28, 2022

I see that every now and then and it's not clear to me what it means. Does reloading the page or devtools help it go away? 🤔

@jerch
Copy link
Member

jerch commented Sep 28, 2022

Oh well found the issue - queueMicrotask in server.js basically bypassed the intermediate buffering. Without the tiny buffer there, websocket creates so many messages, that my browser profiler cannot keep up. Its basically a message congestion from too many tiny messages.
I think we need to keep setTimeout there, but maybe with a smaller limit? Like 2ms instead of 5?

Will see if I can fix it with another PR (had to deal with it anyway in another project), for xterm.js side things seem to be fixed with this PR 👍

@Tyriar
Copy link
Member Author

Tyriar commented Sep 28, 2022

Ah ok, we should tweak that then. Shall we do a very similar change such that we fire the event following incoming data with queueMicrotask/immediate call?

@Tyriar Tyriar merged commit 2659de2 into xtermjs:master Sep 28, 2022
@jerch
Copy link
Member

jerch commented Sep 28, 2022

Ah ok, we should tweak that then. Shall we do a very similar change such that we fire the event following incoming data with queueMicrotask/immediate call?

Yepp, should work there too 😺

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.

serious perf issues on master
2 participants