⚡ Match Highlights: microsoft/terminal/pull/

Chester Liu (skyline75489) · 2019-09-27T12:54:16Z

Summary of the Pull Request

Reduces CPU usage in ATTR_ROW::InsertAttrRuns.

References

PR Checklist

Closes Cursor movement in Vim/Neovim is slower in Terminal #806
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

I myself have experienced the slow cursor movement described in #806 . I found one hotspot is in ATTR_ROW::InsertAttrRuns. It is called many times during screen redrawing. All those vector initializing and copying consumes lots of CPU cycles.

The idea is simple: exit as quickly as possible. The original author has already done this optimization when _list.size() is 1 or 2. I took a little further and aggresive way.

I added quick exit on these following condition

Insert single color that is the same as the left color:

AAAAABBBBBBBCCC
     ^
AAAAAABBBBBBCCC

Insert single color that is the same as surrouding ones

AAAAABBBBBBBCCC
       ^
AAAAABBBBBBBCCC

I didn't change the documentation cuz I wanna hear some feedback from the team before moving on.

Validation Steps Performed

Open .vimrc with vim and see for youself. It's not really "buttery smooth" but it it whole lot better. The CPU usage is dropped by about 30%.

Deleted user (ghost) · 2019-09-27T13:27:48Z

-            if (iStart == left->GetLength() && NewAttr == left->GetAttributes())
+            size_t lowerBound = 0;
+            size_t upperBound = 0;
+            for (int i = 0; i < _list.size(); i++)


std::list::size() return size_t, so this should be:

for (size_t i = 0; i < _list.size(); i++)

Dustin L. Howett (MSFT) (DHowett-MSFT) · 2019-09-28T02:56:56Z

Hey there, thanks for working on this! The engineer who knows the most about this (read: the original author), Michael Niksa (@miniksa), is unfortunately going to be away for a couple business days. Just wanted to let you know that this is on our radar, but feedback may be slow.

Chester Liu (skyline75489) · 2019-09-28T03:07:33Z

Dustin L. Howett (MSFT) (@DHowett-MSFT) Thanks for the heads up. I'll just dig a liittle deeper into this. Cheers for a better terminal 🍺

Deleted user (ghost) · 2019-09-28T06:45:34Z

-            if (iStart == left->GetLength() && NewAttr == left->GetAttributes())
+            size_t lowerBound = 0;
+            size_t upperBound = 0;
+            for (auto i = 0; i < _list.size(); i++)


Use of auto is good, elsewhere, but not in this case. To illustrate the point:

auto i = 0; // deduces to int auto i = 0u; // deduces to unsigned int

The return type of std::list::size() is std::size_t. On x86 builds this is defined as unsigned int, but on x64 builds this is defined as unsigned long long int. So, when building x64, there is a difference of signed vs unsigned and 32-bit integer vs 64-bit integer.

When using auto (deducing to int) this is comparing an int with and unsigned long long int. If the size of the list is > MAX_INT (unlikely to be the case in this particular scenario) this will become an infinite loop.

To prevent unwanted surprises, this is best rewritten as:

for (size_t i = 0; i < _list.size(); i++)

or

for (std::size_t i = 0; i < _list.size(); i++)

Thanks! I came from a Java/C# background and didn't realize this until now.

Dustin L. Howett (MSFT) (DHowett-MSFT) · 2019-09-30T17:45:48Z

/azp run

azure-pipelines · 2019-09-30T17:46:13Z

Azure Pipelines successfully started running 1 pipeline(s).

Dustin L. Howett (MSFT) (DHowett-MSFT) · 2019-10-01T17:47:45Z

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

Michael Niksa (miniksa) · 2019-10-11T22:20:57Z

Chester Liu (@skyline75489), I love the idea and adding the two new cases.

I'd just want you to:

Update the comments in and around the method to very verbosely describe how the method is working and what the cases that it is looking for are
Add tests for your new cases to AttrRowTests in the Host.Tests.Unit library.

Thanks!

Chester Liu (skyline75489) · 2019-10-12T02:43:19Z

Michael Niksa (@miniksa) Glad to hear it from you.

I've checked AttrRowTests and I think that this PR is already covered by TestInsertAttrRunsSingle, if I'm not mistaken. This PR does not break the tests, though. And I'm not sure what I could do now with the tests.

Chester Liu (skyline75489) · 2019-10-12T02:57:21Z

-        else if (_list.size() == 2 && newAttrs.at(0).GetLength() == 1)
+        // .. otherwise if we internally have a list of 2 or more and we're about to insert a single color
+        // it's possible that we just walk left-to-right through the row and find a quick exit.
+        else if (iStart > 0 && iStart == iEnd)


The condition here is, I think, the same as the previous one. But the performance is a lot better because we don't have to access those vector things. TBH I'm surprised by the slowness of vector, even though all you need is just the size().

Michael Niksa (@miniksa) Is there anything that could possibly be broken by the change?

If we stick to the old way, we would be using (_list.size() >= 2 && newAttrs.at(0).GetLength() == 1), just to be clear.

I think it's fine as long as it works. I can't think of anything offhand that would affect the behavior here, so as long as the tests pass and you're satisfied with it... we'll take it and see what happens.

Chester Liu (skyline75489) · 2019-10-12T08:20:18Z

Come to think about it, this PR doesn't really add new cases. It simply broaden the original optimization to make it work when _list.size() is larger than 2. What used to work will still work. What used to be slower is now faster.

Chester Liu (skyline75489) · 2019-10-18T02:37:26Z

Houston, we got a problem. Can someone call /azp to rerun this?

Michael Niksa (miniksa) · 2019-10-18T15:35:16Z

/azp run

azure-pipelines · 2019-10-18T15:35:35Z

Azure Pipelines successfully started running 1 pipeline(s).

Michael Niksa (miniksa) · 2019-10-18T15:36:08Z

If that x64 one doesn't get unstuck with this run, we'll force override it when we have two sign-offs.

Michael Niksa (miniksa) · 2019-10-18T18:08:56Z

-        else if (_list.size() == 2 && newAttrs.at(0).GetLength() == 1)
+        // .. otherwise if we internally have a list of 2 or more and we're about to insert a single color
+        // it's possible that we just walk left-to-right through the row and find a quick exit.
+        else if (iStart > 0 && iStart == iEnd)


I think it's fine as long as it works. I can't think of anything offhand that would affect the behavior here, so as long as the tests pass and you're satisfied with it... we'll take it and see what happens.

Deleted user (ghost) · 2019-10-23T19:55:36Z

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

Handy links:

## Summary of the Pull Request In tonight's episode of "I wanna my CPU back", we'll see a quite familiar face whose name is Mr.AttrRow.  ## References #2937  ## PR Checklist * [ ] Closes #xxx * [ ] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) 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. Issue number where discussion took place: #xxx  ## Detailed Description of the Pull Request / Additional comments I knew this is possible a long time ago. Just didn't got the chance to actually implement this. I understand that you guys are busy preparing the v1.0 release. So if this is a bad time, this can wait.  ## Validation Steps Performed Manually tested. If all goes well, nothing will be broken.

Deleted user (ghost) reviewed Sep 27, 2019

View reviewed changes

Chester Liu (skyline75489) mentioned this pull request Sep 27, 2019

Fix double width cursor for CJK characters #2932

Merged

2 of 5 tasks

Deleted user (ghost) reviewed Sep 28, 2019

View reviewed changes

Chester Liu (skyline75489) changed the title ~~Reduce InsertAttrRuns cpu hotspot~~ Reduce InsertAttrRuns CPU usage Sep 29, 2019

Chester Liu (skyline75489) mentioned this pull request Sep 29, 2019

Reduce text layout CPU usage when DWrite analysis is not needed #2959

Merged

1 of 4 tasks

Mike Griese (zadjii-msft) requested a review from Michael Niksa (miniksa) October 1, 2019 14:23

Mike Griese (zadjii-msft) added Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-Performance Performance-related issue Product-Conhost For issues in the Console codebase labels Oct 1, 2019

Chester Liu (skyline75489) force-pushed the fix/insertRunPerf branch from 812db05 to 99b038f Compare October 1, 2019 23:59

Chester Liu (skyline75489) mentioned this pull request Oct 4, 2019

Reuse text layout for performance #2980

Closed

1 of 5 tasks

Add more quick exit in InsertAttrRuns

2dc2ecf

Chester Liu (skyline75489) force-pushed the fix/insertRunPerf branch from a495923 to 2dc2ecf Compare October 12, 2019 02:51

Chester Liu (skyline75489) commented Oct 12, 2019

View reviewed changes

Michael Niksa (miniksa) added this to the Terminal-1910 milestone Oct 18, 2019

Michael Niksa (miniksa) approved these changes Oct 18, 2019

View reviewed changes

Carlos Zamora (carlos-zamora) approved these changes Oct 18, 2019

View reviewed changes

Carlos Zamora (carlos-zamora) merged commit 60b94a4 into microsoft:master Oct 18, 2019

Chester Liu (skyline75489) deleted the fix/insertRunPerf branch October 20, 2019 05:17

Chester Liu (skyline75489) mentioned this pull request Oct 20, 2019

Break if an early exit can not be found in InsertAttrRuns #3262

Merged

1 of 5 tasks

ghost mentioned this pull request Oct 23, 2019

Cursor movement in Vim/Neovim is slower in Terminal #806

Closed

Chester Liu (skyline75489) mentioned this pull request Apr 29, 2020

Added even more quick exit in InsertAttrRuns #5644

Merged

5 tasks

Conversation

Chester Liu (skyline75489) commented Sep 27, 2019

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

Deleted user (ghost) Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Chester Liu (skyline75489) commented Sep 28, 2019

Uh oh!

Deleted user (ghost) Sep 28, 2019

Choose a reason for hiding this comment

Uh oh!

Chester Liu (skyline75489) Sep 28, 2019

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

azure-pipelines Bot commented Sep 30, 2019

Uh oh!

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

Uh oh!

Michael Niksa (miniksa) commented Oct 11, 2019

Uh oh!

Chester Liu (skyline75489) commented Oct 12, 2019

Uh oh!

Chester Liu (skyline75489) Oct 12, 2019

Choose a reason for hiding this comment

Uh oh!

Chester Liu (skyline75489) Oct 12, 2019

Choose a reason for hiding this comment

Uh oh!

Michael Niksa (miniksa) Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

Chester Liu (skyline75489) commented Oct 12, 2019

Uh oh!

Chester Liu (skyline75489) commented Oct 18, 2019

Uh oh!

Michael Niksa (miniksa) commented Oct 18, 2019

Uh oh!

azure-pipelines Bot commented Oct 18, 2019

Uh oh!

Michael Niksa (miniksa) commented Oct 18, 2019

Uh oh!

Michael Niksa (miniksa) Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

Deleted user (ghost) commented Oct 23, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants