Skip to content

Defer cursor redrawing when writing buffer - #2960

Merged
Dustin L. Howett (MSFT) (DHowett-MSFT) merged 1 commit into
microsoft:masterfrom
skyline75489:fix/delayCursorRedraw
Oct 15, 2019
Merged

Defer cursor redrawing when writing buffer#2960
Dustin L. Howett (MSFT) (DHowett-MSFT) merged 1 commit into
microsoft:masterfrom
skyline75489:fix/delayCursorRedraw

Conversation

@skyline75489

@skyline75489 Chester Liu (skyline75489) commented Sep 29, 2019

Copy link
Copy Markdown
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

Chester Liu (skyline75489) commented Sep 29, 2019

Copy link
Copy Markdown
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

Dustin L. Howett (MSFT) (DHowett-MSFT) commented Sep 30, 2019

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

azure-pipelines Bot commented Sep 30, 2019

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

zadjii-msft
Mike Griese (zadjii-msft) approved these changes Oct 1, 2019
View reviewed changes

@zadjii-msft Mike Griese (zadjii-msft) left a comment

Copy link
Copy Markdown
Member

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 Michael Niksa (@miniksa)'s thoughts though to be sure

@DHowett-MSFT

Dustin L. Howett (MSFT) (DHowett-MSFT) commented Oct 1, 2019

Copy link
Copy Markdown
Contributor

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

This was referenced Oct 4, 2019
Closed
Closed
miniksa
Michael Niksa (miniksa) reviewed Oct 11, 2019
View reviewed changes
Comment thread src/cascadia/TerminalCore/Terminal.cpp
auto& cursor = _buffer->GetCursor();
const Viewport bufferSize = _buffer->GetSize();

cursor.StartDeferDrawing();

@miniksa Michael Niksa (miniksa) Oct 11, 2019

Copy link
Copy Markdown
Contributor

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 Michael Niksa (miniksa) changed the title Defer cursor redrawing when reading buffer Defer cursor redrawing when writing buffer Oct 11, 2019
miniksa
Michael Niksa (miniksa) approved these changes Oct 11, 2019
View reviewed changes

@miniksa Michael Niksa (miniksa) left a comment

Copy link
Copy Markdown
Contributor

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.

@DHowett-MSFT
Dustin L. Howett (MSFT) (DHowett-MSFT) merged commit c905041 into microsoft:master Oct 15, 2019
@DHowett-MSFT

Dustin L. Howett (MSFT) (DHowett-MSFT) commented Oct 15, 2019

Copy link
Copy Markdown
Contributor

Thanks for the contribution!

@skyline75489 Chester Liu (skyline75489) mentioned this pull request Oct 17, 2019
Closed
1 of 5 tasks
@skyline75489
Chester Liu (skyline75489) deleted the fix/delayCursorRedraw branch October 20, 2019 05:17
@ghost

Deleted user (ghost) commented Oct 23, 2019

Copy link
Copy Markdown

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

Handy links:

@nicolaskeller

Nicolas Keller (nicolaskeller) commented Oct 23, 2019

Copy link
Copy Markdown

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

Dustin L. Howett (MSFT) (DHowett-MSFT) commented Oct 23, 2019

Copy link
Copy Markdown
Contributor

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

@skyline75489 Chester Liu (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

@zadjii-msft Mike Griese (zadjii-msft) zadjii-msft approved these changes
+1 more reviewer
@miniksa Michael Niksa (miniksa) miniksa approved these changes
Reviewers whose approvals may not affect merge requirements

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