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

Jump to bottom

Defer cursor redrawing when writing buffer #2960

Merged
merged 1 commit into from
Oct 15, 2019
Merged

Defer cursor redrawing when writing buffer #2960

merged 1 commit into from
Oct 15, 2019

Conversation

skyline75489
Copy link
Collaborator

Summary of the Pull Request

Defer cursor redrawing when reading buffer

References

#2932

PR Checklist

  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan.

Detailed Description of the Pull Request / Additional comments

My previous PR #2932 introduces an overhead when writing a lot of text on screen. I found that this could be optimized by delaying cursor redraw

Validation Steps Performed

@skyline75489
Copy link
Collaborator Author

To take this further, if we know there will be more _WriteBuffer on the way, we can pause the cursor render for a longer time. To my understanding we only need to redraw the cursor when there is no more buffer to print on screen.

@DHowett-MSFT
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@zadjii-msft zadjii-msft requested a review from miniksa October 1, 2019 14:20
zadjii-msft
zadjii-msft approved these changes Oct 1, 2019
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me. I'd definitely want @miniksa's thoughts though to be sure

@DHowett-MSFT
Copy link
Contributor

You will probably need to merge in master to get the CI pipeline to work again. Sorry about that!

@skyline75489 skyline75489 force-pushed the fix/delayCursorRedraw branch from 7dc6179 to 3d42d9d Compare October 1, 2019 23:58
This was referenced Oct 4, 2019
Closed
Closed
miniksa
miniksa reviewed Oct 11, 2019
View reviewed changes
src/cascadia/TerminalCore/Terminal.cpp
@@ -363,6 +363,8 @@ void Terminal::_WriteBuffer(const std::wstring_view& stringView)
auto& cursor = _buffer->GetCursor();
const Viewport bufferSize = _buffer->GetSize();

cursor.StartDeferDrawing();
Copy link
Member

Choose a reason for hiding this comment

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

Leave behind the comment explaining why this is required (for performance reasons, we can not waste time displaying a cursor event when we know more text is coming right behind it).

@miniksa miniksa changed the title Defer cursor redrawing when reading buffer Defer cursor redrawing when writing buffer Oct 11, 2019
miniksa
miniksa approved these changes Oct 11, 2019
View reviewed changes
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

I'm fine with this. It looks like (in historical spelunking of our code) that some of the stream writing methods used to hide the cursor for the same reasons and it was lost to the sands of time.

@skyline75489 skyline75489 force-pushed the fix/delayCursorRedraw branch from 3d42d9d to 7c6bf96 Compare October 12, 2019 03:51
@DHowett-MSFT DHowett-MSFT merged commit c905041 into microsoft:master Oct 15, 2019
@DHowett-MSFT
Copy link
Contributor

Thanks for the contribution!

@skyline75489 skyline75489 mentioned this pull request Oct 17, 2019
Closed
5 tasks
@skyline75489 skyline75489 deleted the fix/delayCursorRedraw branch October 20, 2019 05:17
@ghost
Copy link

ghost commented Oct 23, 2019

🎉Windows Terminal Preview v0.6.2951.0 has been released which incorporates this pull request.:tada:

Handy links:

@nicolaskeller
Copy link

Thank you for the great job so far. Don't know if it refers to this specific topic, but I'm running ntop in PowerShell and even in the new release (Version: 0.6.2951.0) I can see the cursor jumping up and down the screen while the screen gets refreshed. Shouldn't this be fixed by this pull request?

@DHowett-MSFT
Copy link
Contributor

Sorta. You're more likely being impacted by #3093 😄

@skyline75489 skyline75489 mentioned this pull request Jun 10, 2021
Merged
ghost pushed a commit that referenced this pull request Jun 28, 2021
@skyline75489
Throttle cursor redrawing in outputStream.cpp (#10394)
4fc283f 
Try to throttle the cursor redrawing in the conhost world.

The motivation of this is the high CPU usage of `TriggerRedrawCursor` (#10393).

This can be seen as the conhost version of #2960.

This saves 5%~8% of the CPU time.

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

@miniksa miniksa miniksa approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@skyline75489 @DHowett-MSFT @nicolaskeller @miniksa @zadjii-msft