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.

Sign up for GitHub

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

Remove inner setTimeout(), prevents high CPU usage. #1447

Closed
wants to merge 3 commits into from
Closed

Remove inner setTimeout(), prevents high CPU usage. #1447

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9b29360
Remove inner setTimeout(), prevents high CPU usage.
tmpfs May 12, 2018
b2ca055
Prefer requestAnimationFrame() to setTimeout().
tmpfs May 12, 2018
09ef835
Merge branch 'master' into master
May 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Terminal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1290,7 +1290,7 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II
// Kick off a write which will write all data in sequence recursively
this._writeInProgress = true;
// Kick off an async innerWrite so more writes can come in while processing data
setTimeout(() => {
Copy link
Member

Choose a reason for hiding this comment

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

This could delay parsing by 1+ frames. We could maybe just remove this timeout instead?

Copy link
Member

@jerch jerch May 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This cant be removed as it is now, it gives the renderer time to do anything atm. (Removing it will completely disable updates while a command is running, which is reallly fast but prolly not wanted, lol).

Copy link
Member

Choose a reason for hiding this comment

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

This is the timeout that starts a batch, would considering a batch immediately be no good?

Copy link
Member

Choose a reason for hiding this comment

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

For my tests with ls -lR /usr/lib and find / the emulator went silent for a couple of seconds and updated the screen once in a while (like every 3s, for ls it was black until the command had finished). I did not step debug this, seems the updates are way behind without the timeout here.

requestAnimationFrame(() => {
this._innerWrite();
});
}
Expand Down Expand Up @@ -1324,7 +1324,7 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II
}
if (this.writeBuffer.length > 0) {
// Allow renderer to catch up before processing the next batch
setTimeout(() => this._innerWrite(), 0);
Copy link
Member

Choose a reason for hiding this comment

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

This timeout allows the renderer to catch up, without it there may not be a draw for some time. ie. the frame rate can drop significantly

Copy link
Member

@jerch jerch May 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes this change basically reduces screen updates by 90%, now I can count the updates for my typical benchmark with ls -lR /usr/lib. Overall runtime is 10% better with less CPU usage, saved by less time consumed in painting (the greenish pie part). JS runtime is the same (so not the fault of timeout per se).

this._innerWrite();
} else {
this._writeInProgress = false;
}
Expand Down