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

Muffle updates to the cursor position to 1/~100ms #5289

Merged
merged 3 commits into from
Apr 9, 2020

Conversation

DHowett-MSFT
Copy link
Contributor

This stops us from dispatching back-to-back terminal cursor position
updates to the TSF control before it has a chance to get back to us.

Fixes #5288

This was tested live with the 6MB repro file from "Terminal Not Speed Enough".

This stops us from dispatching back-to-back terminal cursor position
updates to the TSF control before it has a chance to get back to us.

Fixes #5288.
miniksa
miniksa previously approved these changes Apr 8, 2020
@miniksa
Copy link
Member

miniksa commented Apr 8, 2020

I still wanted it to be named "tuned mass damper" but fine. Let's do this.

leonMSFT
leonMSFT previously approved these changes Apr 8, 2020
Copy link
Contributor

@leonMSFT leonMSFT left a comment

Choose a reason for hiding this comment

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

Thanks for doing this 😅

@DHowett-MSFT
Copy link
Contributor Author

We still dispatch about 60 of them printing the entire 6MB file, but that's a 100000000000000000% reduction and the UI remains fully responsive.

@DHowett-MSFT
Copy link
Contributor Author

Alright, i switched this from a mutex to an atomic -- it's cheaper and the platform cannot deny us.

I also took some measurements. For a .070-second run of trace logging from that 6MB file:

We tried to notify TSF 19798 times, and we successfully notified it 14 times.

We can probably do better than "notify 14 times in 7 hundredths of a second"

@DHowett-MSFT DHowett-MSFT dismissed stale reviews from leonMSFT and miniksa April 8, 2020 23:45

changing

@DHowett-MSFT
Copy link
Contributor Author

After damping, i got it down to 502 cursor updates for the entire file print.

@DHowett-MSFT DHowett-MSFT force-pushed the dev/duhowett/put_a_sock_in_it branch from e8f95d2 to 93a2995 Compare April 8, 2020 23:56
Copy link
Contributor

@leonMSFT leonMSFT left a comment

Choose a reason for hiding this comment

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

Makes sense to me, seems to muffle harder

@DHowett-MSFT DHowett-MSFT changed the title Muffle updates to the cursor position Muffle updates to the cursor position to 1/~100ms Apr 9, 2020
@DHowett-MSFT DHowett-MSFT merged commit 1299a83 into master Apr 9, 2020
@DHowett-MSFT DHowett-MSFT deleted the dev/duhowett/put_a_sock_in_it branch April 9, 2020 00:20
@zadjii-msft
Copy link
Member

Hey I'm just seeing this now in the morning, but @greg904 had a great little throttle helper over in TimeThrottle.h in #4608 that might be able to be adapted for this to dampen even further

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.

TerminalCore/TerminalControl dispatch five billion cursor update coroutines when printing long file
5 participants