Reduce InsertAttrRuns CPU usage #2937

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%.

encladeus · 2019-09-27T13:27:48Z

src/buffer/out/AttrRow.cpp

-            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++)

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), @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.

skyline75489 · 2019-09-28T03:07:33Z

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

encladeus · 2019-09-28T06:45:34Z

src/buffer/out/AttrRow.cpp

-            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.

DHowett-MSFT · 2019-09-30T17:45:48Z

/azp run

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

Azure Pipelines successfully started running 1 pipeline(s).

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!

miniksa · 2019-10-11T22:20:57Z

@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!

skyline75489 · 2019-10-12T02:43:19Z

@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.

skyline75489 · 2019-10-12T02:57:21Z

src/buffer/out/AttrRow.cpp

-        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().

@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.

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.

skyline75489 · 2019-10-18T02:37:26Z

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

miniksa · 2019-10-18T15:35:16Z

/azp run

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

Azure Pipelines successfully started running 1 pipeline(s).

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.

miniksa · 2019-10-18T18:08:56Z

src/buffer/out/AttrRow.cpp

-        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.

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.

encladeus reviewed Sep 27, 2019

View reviewed changes

skyline75489 mentioned this pull request Sep 27, 2019

Fix double width cursor for CJK characters #2932

Merged

5 tasks

encladeus reviewed Sep 28, 2019

View reviewed changes

skyline75489 changed the title ~~Reduce InsertAttrRuns cpu hotspot~~ Reduce InsertAttrRuns CPU usage Sep 29, 2019

skyline75489 mentioned this pull request Sep 29, 2019

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

Merged

4 tasks

zadjii-msft requested a review from miniksa October 1, 2019 14:23

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

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

skyline75489 mentioned this pull request Oct 4, 2019

Reuse text layout for performance #2980

Closed

5 tasks

Add more quick exit in InsertAttrRuns

2dc2ecf

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

skyline75489 commented Oct 12, 2019

View reviewed changes

miniksa added this to the Terminal-1910 milestone Oct 18, 2019

miniksa approved these changes Oct 18, 2019

View reviewed changes

carlos-zamora approved these changes Oct 18, 2019

View reviewed changes

carlos-zamora merged commit 60b94a4 into microsoft:master Oct 18, 2019

skyline75489 deleted the fix/insertRunPerf branch October 20, 2019 05:17

skyline75489 mentioned this pull request Oct 20, 2019

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

Merged

5 tasks

ghost mentioned this pull request Oct 23, 2019

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

Closed

skyline75489 mentioned this pull request Apr 29, 2020

Added even more quick exit in InsertAttrRuns #5644

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce InsertAttrRuns CPU usage #2937

Reduce InsertAttrRuns CPU usage #2937

skyline75489 commented Sep 27, 2019

encladeus Sep 27, 2019

DHowett-MSFT commented Sep 28, 2019

skyline75489 commented Sep 28, 2019

encladeus Sep 28, 2019

skyline75489 Sep 28, 2019

DHowett-MSFT commented Sep 30, 2019

azure-pipelines bot commented Sep 30, 2019

DHowett-MSFT commented Oct 1, 2019

miniksa commented Oct 11, 2019

skyline75489 commented Oct 12, 2019

skyline75489 Oct 12, 2019

skyline75489 Oct 12, 2019

miniksa Oct 18, 2019

skyline75489 commented Oct 12, 2019

skyline75489 commented Oct 18, 2019

miniksa commented Oct 18, 2019

azure-pipelines bot commented Oct 18, 2019

miniksa commented Oct 18, 2019

miniksa Oct 18, 2019

ghost commented Oct 23, 2019

Reduce InsertAttrRuns CPU usage #2937

Reduce InsertAttrRuns CPU usage #2937

Conversation

skyline75489 commented Sep 27, 2019

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

encladeus Sep 27, 2019

Choose a reason for hiding this comment

DHowett-MSFT commented Sep 28, 2019

skyline75489 commented Sep 28, 2019

encladeus Sep 28, 2019

Choose a reason for hiding this comment

skyline75489 Sep 28, 2019

Choose a reason for hiding this comment

DHowett-MSFT commented Sep 30, 2019

azure-pipelines bot commented Sep 30, 2019

DHowett-MSFT commented Oct 1, 2019

miniksa commented Oct 11, 2019

skyline75489 commented Oct 12, 2019

skyline75489 Oct 12, 2019

Choose a reason for hiding this comment

skyline75489 Oct 12, 2019

Choose a reason for hiding this comment

miniksa Oct 18, 2019

Choose a reason for hiding this comment

skyline75489 commented Oct 12, 2019

skyline75489 commented Oct 18, 2019

miniksa commented Oct 18, 2019

azure-pipelines bot commented Oct 18, 2019

miniksa commented Oct 18, 2019

miniksa Oct 18, 2019

Choose a reason for hiding this comment

ghost commented Oct 23, 2019