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

Optimize critical I/O path for input latency #4145

Merged
merged 2 commits into from Sep 25, 2022

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Sep 24, 2022

This reduced input latency from about 10ms -> 4ms on my mac (measurement based on devtools tasks).

Before server microtask:

Screen Shot 2022-09-24 at 4 23 02 pm

After server microtask:

Screen Shot 2022-09-24 at 4 22 38 pm

After server microtask and write buffer microtask:

Screen Shot 2022-09-24 at 4 26 41 pm

This reduces demo input latency on my mac approx 10ms -> 4ms
This seems to make little difference but it's more correct as we want
to write as fast as possible to minimize input latency
@Tyriar Tyriar added this to the 5.1.0 milestone Sep 24, 2022
@Tyriar Tyriar self-assigned this Sep 24, 2022
@Tyriar Tyriar merged commit 318abe6 into xtermjs:master Sep 25, 2022
Tyriar added a commit to microsoft/vscode that referenced this pull request Sep 26, 2022
- Optimize critical I/O path for input latency xtermjs/xterm.js#4145
- Add PriorityTaskQueue xtermjs/xterm.js#4144
- Use the browser's IdleDeadline to determine max task time xtermjs/xterm.js#4143
- Defer paused renderer resize to idle callback xtermjs/xterm.js#4142
- Do char atlas warmup via new IdleTaskQueue xtermjs/xterm.js#4141
- Share rgba vars throughout Color.ts, fast setTheme parseColor xtermjs/xterm.js#4140
- Optimize contexts for reading frequently xtermjs/xterm.js#4137
- Fix width of upper 1/8 block char xtermjs/xterm.js#4134

Fixes #161323
Copy link
Member

@jerch jerch left a comment

Choose a reason for hiding this comment

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

@Tyriar I am not so sure about queueMicrotask usage here - see comments below.

@@ -105,7 +105,7 @@ export class WriteBuffer {
// schedule chunk processing for next event loop run
if (!this._writeBuffer.length) {
this._bufferOffset = 0;
setTimeout(() => this._innerWrite());
queueMicrotask(() => this._innerWrite());
Copy link
Member

Choose a reason for hiding this comment

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

This should be safe with queueMicrotask - reason: this code path is only entered, if the write queue was empty before, which guarantees, that there happened a screen refresh before.

@@ -217,7 +217,7 @@ export class WriteBuffer {
this._callbacks = this._callbacks.slice(this._bufferOffset);
this._bufferOffset = 0;
}
setTimeout(() => this._innerWrite());
queueMicrotask(() => this._innerWrite());
Copy link
Member

@jerch jerch Sep 27, 2022

Choose a reason for hiding this comment

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

Not so sure about queueMicrotask here - imho this will skip any screen refreshs in between, if the write queue never drops empty? (This edge case might be provokable by tiny messages at sync promisified term.write calls while the write queue never drops empty...)

There are more edge cases linked to direct microtask usage - imho chrome and firefox prioritize them differently (dont remember the details atm).

Copy link
Member Author

Choose a reason for hiding this comment

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

Opps. You're right, will fix

There are more edge cases linked to direct microtask usage - imho chrome and firefox prioritize them differently (dont remember the details atm).

I didn't test in Firefox but this should only matter if queueMicrotask ends up slower than setTimeout which I think would be against the spec?

Tyriar added a commit to Tyriar/xterm.js that referenced this pull request Sep 27, 2022
lemanschik pushed a commit to code-oss-dev/code that referenced this pull request Nov 25, 2022
- Optimize critical I/O path for input latency xtermjs/xterm.js#4145
- Add PriorityTaskQueue xtermjs/xterm.js#4144
- Use the browser's IdleDeadline to determine max task time xtermjs/xterm.js#4143
- Defer paused renderer resize to idle callback xtermjs/xterm.js#4142
- Do char atlas warmup via new IdleTaskQueue xtermjs/xterm.js#4141
- Share rgba vars throughout Color.ts, fast setTheme parseColor xtermjs/xterm.js#4140
- Optimize contexts for reading frequently xtermjs/xterm.js#4137
- Fix width of upper 1/8 block char xtermjs/xterm.js#4134

Fixes microsoft#161323
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.

None yet

2 participants