Defer _run substr operation in StateMachine::ProcessString #10471

skyline75489 · 2021-06-21T09:01:08Z

Do not eagerly create run by substring to reduce the unnecessary overhead.

Switching from .substr to offset/length tracking saves us 7% CPU time.

skyline75489 · 2021-06-21T09:02:46Z

This saves ~7% of CPU time

Before:

After:

skyline75489 · 2021-06-21T10:29:01Z

src/terminal/parser/stateMachine.cpp

    while (current < string.size())
    {
        // The run will be everything from the start INCLUDING the current one
        // in case we process the current character and it turns into a passthrough
        // fallback that picks up this _run inside `FlushToTerminal` above.
-        _run = string.substr(start, current - start + 1);


Repeatedly substring turns out to be quite expensive and takes up about 10%-12% of the total CPU time.

I honestly forgot whether the 10% here is in OpenConsole or WT, or both...

I feel like you should leave the comment/detail somewhere in here that we shouldn't get the future-bright-idea to "improve safety" here by re-wrapping this into a convenience class. It needs to stay as a raw sort of offset thing because of how high performance this needs to be.

skyline75489 · 2021-06-21T10:35:18Z

src/terminal/parser/stateMachine.cpp

@@ -1838,12 +1839,17 @@ void StateMachine::ProcessString(const std::wstring_view string)
    size_t start = 0;
    size_t current = start;

+    _currentString = string;


Do I need to reset these in _EnterGround ? Feels like I should.

I'm not sure about this any more. Because ResetState() is also called inside ProcessString.

I see what you were saying, but I agree this appears as a valid exception.

src/terminal/parser/stateMachine.hpp

lhecker

You can make _run a local variable inside StateMachine::ProcessString.
This removes the _run member entirely and removes the redundant state from this class. Can't forget to call _ActuateCurrentRun, if there's nothing to forget, right?

src/terminal/parser/stateMachine.hpp

src/terminal/parser/stateMachine.cpp

skyline75489 · 2021-06-26T04:05:01Z

Thanks @lhecker for the wonderful idea!

Note that StateMachine::ProcessString is used heavily in both OpenConsole & WT. I'd expect noticeable performance
improvement in the official build, which is PGO enhanced. My local build isn't with PGO, so the improvement is limited.

skyline75489 · 2021-06-26T04:19:33Z

src/terminal/parser/stateMachine.cpp

@@ -1885,14 +1891,23 @@ void StateMachine::ProcessString(const std::wstring_view string)
    // When we leave the loop, current has been advanced to the length of the string itself
    // (or one past the array index to the final char) so this `substr` operation doesn't +1
    // to include the final character (unlike the one inside the top of the loop above.)
-    _run = start < string.size() ? string.substr(start) : std::wstring_view{};
+    if (start < string.size())


I don't quite understand this piece of comment above, be it before this PR or after. Still the logic here remains intact.

It's saying that on line 1846, a .substr is called with the 2nd parameter having a +1 on the end as a magic value that helps select the correct distance.

So one might quickly think that this similar .substr call on line 1888 should also have a +1 magic value and there's an error. But then it goes on to clarify that the nature of the looping made the magic +1 unnecessary.

Or at least that's my understanding of the comment. It looks like this code may have been revised a few times since it was originally written.

skyline75489 · 2021-06-26T04:31:29Z

Strange. I can't repro the feature tests failure locally. Maybe it's transient? Also the x86 tests are OK.

skyline75489 · 2021-06-27T04:32:25Z

OK the test failures seems to be transient. Merged main and now the CI is green.

lhecker

Just one comment.

src/terminal/parser/stateMachine.cpp

DHowett

A string_view is just a pointer and a size; can you go into some detail on why exactly this is so much more performant? Is it just the repeated bounds checks in substr that the language standard requires? There must be a better way to deal with this than to move back to counts and offsets...

lhecker · 2021-06-27T22:39:20Z

@DHowett This is a CPU trace of OpenConsole when running termbench:

I've benchmarked this patch on my experimental, fast glyph-atlas renderer and it improves VT performance from 490 kcg/s up to 550 kcg/s, or a whopping +12% just like that!
(To anyone reading this: Yes, 550 kcg/s isn't comparatively fast, but the cause for this is simply the mere lack of proper double buffering.)

DHowett · 2021-06-27T22:41:10Z

Right, but substr is a simple operation. What's inside it that's so expensive?

skyline75489 · 2021-06-27T22:55:03Z

That single line is combination of: substr, constructing a string_view, and copying a string_view into _run. You want me to find a winner among these three? 😅

Talked with Leonard. He thinks it’s substr with all the internal checks for boundaries and everything.

lhecker · 2021-06-27T22:56:29Z

@DHowett Benchmarking suggests that the removal of .at() improves performance by just 10 kcg/s (+2%) and the removal of .substr() by 50 kcg/s (+10%).
I believe this is caused by inoptimal assembly generated due to .substr(), causing conditional moves and jumps to be emitted. Additionally I believe that exception-throwing code paths cause the CPU pipeline to be less effective.

lhecker · 2021-06-27T23:05:22Z

The difference in assembly is basically (shortened)....

Before:

; exception handling for .substr()
cmp     rsi, rbx
jb      SHORT $LN27@ProcessStr 
; calling ProcessCharacter (part 1)
mov     rcx, QWORD PTR [r14]
; bad substr assembly
mov     r8, rdi
sub     r8, rbx
mov     rdx, rsi
inc     r8
sub     rdx, rbx
cmp     rdx, r8
lea     rax, QWORD PTR [rcx+rbx*2]
cmovb   r8, rdx
mov     QWORD PTR $T1[rsp], rax
mov     QWORD PTR $T1[rsp+8], r8
movups  xmm0, XMMWORD PTR $T1[rsp]                                                        ; < slow (~5 cycles latency)
movups  XMMWORD PTR std::basic_string_view<wchar_t,std::char_traits<wchar_t> > _run, xmm0 ; < slow (another ~5 cycles)
; exception handling for .at()
cmp     rsi, rdi
jbe     SHORT $LN27@ProcessStr
; calling ProcessCharacter (part 2)
movzx   ecx, WORD PTR [rcx+rdi*2]
call    ProcessCharacter(wchar_t)

After:

; nice counting stuff
mov     QWORD PTR unsigned __int64 _runOffset, rbx
mov     rax, rdi
sub     rax, rbx
inc     rax
mov     QWORD PTR unsigned __int64 _runSize, rax
; calling ProcessCharacter
movzx   ecx, WORD PTR [rsi]
call    ProcessCharacter(wchar_t)

skyline75489 · 2021-06-28T02:56:08Z

Wow after seeing the assembly code posted by @lhecker , I'm not sure if there is "a better way to deal with this than to move back to counts and offsets". Not try to brag here, but the "counting stuff" looks so much better!

DHowett · 2021-06-28T15:36:42Z

Thanks for the explanation 😄

DHowett · 2021-06-28T15:37:31Z

@msftbot make sure @miniksa signs off

ghost · 2021-06-28T15:37:36Z

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

src/terminal/parser/stateMachine.cpp

src/terminal/parser/stateMachine.hpp

miniksa

If it makes things faster, I'm all over it. I just want you to try to make sure that someone doesn't lose this again in the future when they forget this particular lesson and go on a memory safety campaign.

skyline75489 · 2021-06-29T08:38:09Z

With this PR, together with the previous ones that has been merged into main, I'm seeing 80% performance boost in OpenConsole (88kcg) comparing to cmd.exe (19043.1052) (50kcg), and this is only my local build!

ghost · 2021-06-29T14:10:14Z

Hello @lhecker!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Chester addressed Michael's concerns.

Do not eagerly create run by substring to reduce the unnecessary overhead. Switching from `.substr` to offset/length tracking saves us 7% CPU time. (cherry picked from commit 6a37818)

ghost · 2021-07-14T17:02:08Z

🎉Windows Terminal v1.9.1942.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2021-07-14T17:06:26Z

🎉Windows Terminal Preview v1.10.1933.0 has been released which incorporates this pull request.:tada:

Handy links:

Defer _run substr operation in ProcessString

fa51f96

skyline75489 added the Area-Performance Performance-related issue label Jun 21, 2021

ActuateCurrentRun

2143aaf

skyline75489 commented Jun 21, 2021

View reviewed changes

skyline75489 changed the title ~~Defer _run substr operation in ProcessString~~ Defer _run substr operation in StateMachine::ProcessString Jun 21, 2021

skyline75489 commented Jun 21, 2021

View reviewed changes

src/terminal/parser/stateMachine.hpp Outdated Show resolved Hide resolved

til::at

93d2e91

lhecker reviewed Jun 25, 2021

View reviewed changes

src/terminal/parser/stateMachine.hpp Outdated Show resolved Hide resolved

src/terminal/parser/stateMachine.cpp Outdated Show resolved Hide resolved

src/terminal/parser/stateMachine.cpp Outdated Show resolved Hide resolved

Refactor; Kill _run

79d61fe

skyline75489 commented Jun 26, 2021

View reviewed changes

Raud-pi approved these changes Jun 26, 2021

View reviewed changes

Merge branch 'main' into chesterliu/dev/state-machine

b261183

lhecker approved these changes Jun 27, 2021

View reviewed changes

src/terminal/parser/stateMachine.cpp Outdated Show resolved Hide resolved

DHowett reviewed Jun 27, 2021

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 28, 2021

DHowett requested changes Jun 28, 2021

View reviewed changes

src/terminal/parser/stateMachine.cpp Outdated Show resolved Hide resolved

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 28, 2021

miniksa reviewed Jun 28, 2021

View reviewed changes

src/terminal/parser/stateMachine.hpp Show resolved Hide resolved

miniksa previously requested changes Jun 28, 2021

View reviewed changes

skyline75489 added 2 commits June 29, 2021 09:02

Review

52685b6

Merge branch 'main' into chesterliu/dev/state-machine

56cdc28

ghost removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something AutoMerge Marked for automatic merge by the bot when requirements are met labels Jun 29, 2021

lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 29, 2021

DHowett approved these changes Jun 29, 2021

View reviewed changes

DHowett merged commit 6a37818 into microsoft:main Jun 29, 2021

skyline75489 mentioned this pull request Jul 6, 2021

Extremely slow performance when processing virtual terminal sequences #10462

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Defer _run substr operation in StateMachine::ProcessString #10471

Defer _run substr operation in StateMachine::ProcessString #10471

skyline75489 commented Jun 21, 2021 •

edited by DHowett

Loading

skyline75489 commented Jun 21, 2021

skyline75489 Jun 21, 2021 •

edited

Loading

skyline75489 Jun 26, 2021

miniksa Jun 28, 2021

skyline75489 Jun 21, 2021

skyline75489 Jun 26, 2021

miniksa Jun 28, 2021

lhecker left a comment

skyline75489 commented Jun 26, 2021

skyline75489 Jun 26, 2021 •

edited

Loading

miniksa Jun 28, 2021

skyline75489 commented Jun 26, 2021 •

edited

Loading

skyline75489 commented Jun 27, 2021

lhecker left a comment

DHowett left a comment

lhecker commented Jun 27, 2021

DHowett commented Jun 27, 2021

skyline75489 commented Jun 27, 2021

lhecker commented Jun 27, 2021

lhecker commented Jun 27, 2021 •

edited

Loading

skyline75489 commented Jun 28, 2021

DHowett commented Jun 28, 2021

DHowett commented Jun 28, 2021

ghost commented Jun 28, 2021

miniksa left a comment

skyline75489 commented Jun 29, 2021 •

edited

Loading

ghost commented Jun 29, 2021

ghost commented Jul 14, 2021

ghost commented Jul 14, 2021

Defer _run substr operation in StateMachine::ProcessString #10471

Defer _run substr operation in StateMachine::ProcessString #10471

Conversation

skyline75489 commented Jun 21, 2021 • edited by DHowett Loading

skyline75489 commented Jun 21, 2021

skyline75489 Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

skyline75489 Jun 26, 2021

Choose a reason for hiding this comment

miniksa Jun 28, 2021

Choose a reason for hiding this comment

skyline75489 Jun 21, 2021

Choose a reason for hiding this comment

skyline75489 Jun 26, 2021

Choose a reason for hiding this comment

miniksa Jun 28, 2021

Choose a reason for hiding this comment

lhecker left a comment

Choose a reason for hiding this comment

skyline75489 commented Jun 26, 2021

skyline75489 Jun 26, 2021 • edited Loading

Choose a reason for hiding this comment

miniksa Jun 28, 2021

Choose a reason for hiding this comment

skyline75489 commented Jun 26, 2021 • edited Loading

skyline75489 commented Jun 27, 2021

lhecker left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

lhecker commented Jun 27, 2021

DHowett commented Jun 27, 2021

skyline75489 commented Jun 27, 2021

lhecker commented Jun 27, 2021

lhecker commented Jun 27, 2021 • edited Loading

skyline75489 commented Jun 28, 2021

DHowett commented Jun 28, 2021

DHowett commented Jun 28, 2021

ghost commented Jun 28, 2021

miniksa left a comment

Choose a reason for hiding this comment

skyline75489 commented Jun 29, 2021 • edited Loading

ghost commented Jun 29, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Jul 14, 2021

ghost commented Jul 14, 2021

skyline75489 commented Jun 21, 2021 •

edited by DHowett

Loading

skyline75489 Jun 21, 2021 •

edited

Loading

skyline75489 Jun 26, 2021 •

edited

Loading

skyline75489 commented Jun 26, 2021 •

edited

Loading

lhecker commented Jun 27, 2021 •

edited

Loading

skyline75489 commented Jun 29, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.