Use memcmp for TextAttribute & TextColor comparison

[skyline75489]

TextAttribute and TextColor are commonly used structures in hot paths.
This commit replaces more complex comparisons where each field is compared
independently with a single call to memcmp. This compiles down to just
a few instructions. This reduces code and binary size and improves
performance for paths were TextAttributes need to be compared.

PR Checklist

• Supports [Performance] vtebench tracking issue #10563
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed
• Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
• Schema 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

Validation Steps Performed

• termbench still works ✔️

Co-authored-by: Leonard Hecker lhecker@microsoft.com

[DHowett]

This feels a lot like a micro-optimization... doing one comparison instead of four and growing the non-default case to five comparisons doesn't seem like it's going to win us any races.

Do you have profiling data for this change?

If this really is a micro-optimization we want to take on, I'd rather it be using memcmp or similar. It optimizes down to a branchless 4-byte comparison under /O2.

bool operator==(TextColor const &,TextColor const &) PROC               ; operator==, COMDAT
        mov     eax, DWORD PTR [rcx]
        cmp     eax, DWORD PTR [rdx]
        sete    al
        ret     0

/cc @lhecker

[DHowett]

(under ARM64 is optimizes down to an actual call to memcmp. To be clear: i am not recommending we do this)

[lhecker]

+1 for using memcmp on primitive types.
Technically it will not be legal for this struct (because it has a constructor and thus isn’t a so called trivial type), but it‘ll be fine anyways for our use case.

[DHowett]

If these 10 comparisons are a significant burden via our compiler, I'm blown away.

[j4james]

This is from godbolt for the following demo code (all under O3):

If you were in fact using /O3, that's probably your problem. That's not a valid option as far as I'm aware, so it's likely just ignored. I believe you need /O2 for maximum optimizations.

[skyline75489]

Yeah maybe. I am very new to godbolt and the assembly word. Please don’t hesitate to point out any mistakes I made. 获取 Outlook for iOS<https://aka.ms/o0ukef>

[j4james]

For the record, I'm also +1 on using memcmp if it makes a significant difference. If we're worried about the correctness of it, could we not throw in a static_assert or something that verifies the class size is what we expect it to be for the given field sizes? Hopefully then there's less risk of something breaking unexpectedly as a result of packing/alignment changes.

[skyline75489]

This is to make std::has_unique_object_representations_v<TextAttribute> happy.

[serprex]

Wouldn't it be better to keep this as BYTE & add a uint8_t zeropad to TextAttribute?

[lhecker]

That's a good idea! If @skyline75489 wants to he could still change it. 🙂
Personally I'd be fine merging it either way though.

[skyline75489]

OK, the actual performance gain from this PR is very limited. But I still like some of the tricks in it. So feel free to merge this or leave it. I'm moving to #10596 from now on.

[DHowett]

OK, the actual performance gain from this PR is very limited.

I dunno if I love the tricks given that it doesn't net us much gain. It's cool that we're using C++ fixtures to ensure that we're not doing something ~ ~ weird ~ ~, but... perhaps our optimization talent is better-spent somewhere else?

[skyline75489]

@DHowett I was wrong the performance gain from this PR being very limited. With the initial work in #10596 done on my experimental branch, this PR leads to ~10%+ in throughput (3.6s -> 3.3s). And even after the fix the comparison is still somewhat expensive.

Let's just leave the PR here and I can come back to this later.

[skyline75489]

@lhecker do you want this? I was going to close this,

[lhecker]

@skyline75489 I'm personally still in favor of this PR, but I believe it has to wait for 1.12.

[EmJayGee]

OK well that's interesting. In any case, looking at the original operator == implementation, the fields are not compared in "natural" order and since && is short-circuiting, it's vaguely possible that the optimizer can't see past the weird order of field access. For some reason the project isn't building right on my machine which is a separate issue I'm working through but you might try just rewriting operator== as...

constexpr bool operator==(const TextAttribute& a, const TextAttribute& b) noexcept
{
return a._wAttrLegacy == b._wAttrLegacy &&
a._hyperlinkId == b._hyperlinkId &&
a._foreground == b._foreground &&
a._background == b._background &&
a._extendedAttrs == b._extendedAttrs;
}

(basically comparison of _hyperlinkId was at the end instead of second as per the layout)

Independently, it seems like you have a basic correctness problem with the sizeof() based memcmp solution. The sizeof(TextAttribute) is going to be 16 but as far as I can tell only 13 bytes are filled in by explicit field references (unless UNIT_TESTING is defined but still then there's a big alignment hole, the contents of which are uninitialized).

Fortunately since it's final there's no vtable so you could play the memset(this, 0, sizeof(*this)) game in the constructor but this is really going down a worse path. I think you should coerce the compiler to do the right thing without any of these games. (This could break whatever the stuff is under UNIT_TESTING, if it has vtables, the memset() technique famously will screw that up badly.)

I'm trying to find the definitions of TextColor and ExtendedAttributes to understand them better. TextColor seems nontrivial (given what I saw in the TextAttribute constructor) and may be part of the problem.

[lhecker]

In any case, looking at the original operator == implementation, the fields are not compared in "natural" order and since && is short-circuiting, it's vaguely possible that the optimizer can't see past the weird order of field access.

Good idea! I tried that and it didn't work unfortunately. I also tried whether the spaceship operator <=> is a viable alternative to generate optimal assembly and unfortunately that's not the case.
And I just realized that C doesn't have native comparisons for structs either.

I've put all of them together into a single comparison: https://godbolt.org/z/5qdKxPq4f
The top row is written in C++ and the bottom one in C.
The left compilation is MSVC and the right one clang.

Independently, it seems like you have a basic correctness problem with the sizeof() based memcmp solution. The sizeof(TextAttribute) is going to be 16 but as far as I can tell only 13 bytes are filled in by explicit field references (unless UNIT_TESTING is defined but still then there's a big alignment hole, the contents of which are uninitialized).

The struct has an alignment of 2 and this PR changes the last member to be 2 bytes large. That way the struct should have a sizeof of 14 with an alignment of 2. I think this should work correctly with memcmp...

I'm trying to find the definitions of TextColor and ExtendedAttributes to understand them better.

TextColor can be found here: https://github.com/microsoft/terminal/blob/main/src/buffer/out/TextColor.h
and ExtendedAttributes here: https://github.com/microsoft/terminal/blob/main/src/inc/conattrs.hpp

TextColor seems nontrivial (given what I saw in the TextAttribute constructor) and may be part of the problem.

I was wondering the same... If I remove the TextColor members from the struct and comparison operators, then the "native" comparison still involves a bunch of jumps apparently.

[lhecker]

Ah btw: std::has_unique_object_representations_v<T>, as used in this PR, would return false for any structs that require padding AFAIK.
The description of std::has_unique_object_representations_v is basically: "returns true if every bit in the given type contributes to its uniqueness" and it was added for people writing hash functions, so that they can safely cast T to an array of bytes and hash each byte without worrying about uninitialized padding memory, etc.
In this case we abuse it to ensure we can safely compare every byte in T.

[EmJayGee]

Thanks I'll figure out my build problems and tinker with this and have some more concrete recommendations. I used to be quite adept at getting the msvc compiler to generate good code in cases like this, I'm getting weird access denied errors in the build. Is there some forum for asking about problems like this? (I'll hold back from obvious spitballing of possible problems since this is not the right context.)

[lhecker]

@EmJayGee Feel free to create an issue or a discussion right here on GitHub, with screenshots or text outputs of the issue you're facing. I'm sure I'm speaking for everyone on the team when I say that we'd be happy to help you set up the project. 🙂

[EmJayGee]

What gives you the impression that the struct/class has an alignment (packing?) of 2? I don't see any packing/alignment indications. I've been away from the code for a while so perhaps the norms have changed here but I would expect 8 without coercion. Small members may be packed within it on smaller boundaries but maybe I lost context somewhere. Is there a projectwide setting or something? That would, in general, be a performance problem since 2-byte alignment, in general, is not performant for most 4 byte and larger objects.

If you have these assumptions, I would definitely encode them in compile time asserts. I see comments on the data members which perhaps are in a format that some linter recognizes but nothing beats the compiler doing the verification itself.

My C++-fu is about 7 years out of date so I need to begin my training over, time to put on the weight vest and enter the gravity chamber.

[lhecker]

What gives you the impression that the struct/class has an alignment (packing?) of 2? I don't see any packing/alignment indications.

The first member, _wAttrLegacy, is a uint16_t which has an alignment of 2. The remaining members are chosen in such a way as to make sure that no padding bytes are inserted.

I've added the following static (compile-time) assertions a while ago, ensuring this doesn't get accidentally changed:

terminal/src/buffer/out/TextAttribute.cpp

Lines 8 to 13 in f588223

	// Keeping TextColor compact helps us keeping TextAttribute compact,
	// which in turn ensures that our buffer memory usage is low.
	static_assert(sizeof(TextAttribute) == 14);
	static_assert(alignof(TextAttribute) == 2);
	// Ensure that we can memcpy() and memmove() the struct for performance.
	static_assert(std::is_trivially_copyable_v<TextAttribute>);

(I've put the assertion in the .cpp file, because the corresponding header gets included in way too many files, leading to unnecessary recompilations on every change. No one had time to clean that up yet unfortunately.)

When we run-length-encode our TextAttribute arrays we pack them into an array of

struct {
	TextAttribute value;
	uint16_t length;
};

elements, which given the sizeof(TextAttribute) == 14 and alignof(TextAttribute) == 2 results in nice, even 16 byte structures for the RLE compressed array. (I just realized we should probably do an aligned heap alloc with an alignment of 16 for the structure, so that it doesn't straddle cache lines...)

[EmJayGee]

I don't know what your portability requirements are but on Windows, the heap allocators will always give you cache line aligned allocations so you'll be good. Well I left before the arm64 stuff became mainstream so I don't know what the deal is there but you're good to go on the intel/amd architectures.

I can do a PR for moving the static asserts to the headers. I'm trying to get my coding legs back especially in this new world of GIT so I'll see what I can whip together for that next week.

Can you point me to where the disassemble the code in situ (binary, what function references the equality operator where we would ideally see an inlined optimized comparison) to see the generated code best? And/or what kind of workload you did to measure the original issue so I can repro it with the current windows perf toolkit tools, see the issue there so I can push the comparison operators around and observe how my changes change things?

Thanks, Mike

[lhecker]

Can you point me to where the disassemble the code in situ (binary, what function references the equality operator where we would ideally see an inlined optimized comparison) to see the generated code best?

I'm not sure I understand what you mean... If you're asking were TextAttributes are compared, then it'd primarily be this line as far as I'm aware:

terminal/src/renderer/base/renderer.cpp

Line 828 in f588223

if (color != it->TextAttr() || changedPatternOrFont)

There you should be able to view the disassembled code.

And/or what kind of workload you did to measure the original issue so I can repro it with the current windows perf toolkit tools, see the issue there so I can push the comparison operators around and observe how my changes change things?

You can use my rainbowbench. It'll draw a rainbow as quickly as possible and tell you the resulting "kcg/s", or in other words how many thousands (k) of colored (c) glyphs (g) it can draw per second (s). Since every character has a unique color, it'll result in every character having a different TextAttribute and thus involve many such comparisons.

[EmJayGee]

@lhecker, sorry I was off doing other things (learning git, github practicing by checking in a trivial build break fix for retail builds) but I'm back now. Your link to rainbowbench leads to a 404 not found. Can you point me to another?

In terms of where to look, I mean like a good function to disassemble to see it inlined. Just going to the equality comparison function in question isn't necessarily particularly interesting since that won't show what the inliner does in context.

So _PaintBufferOutputHelper() is the hot function? Do we have any output from the various perf tools to peruse?

[lhecker]

@EmJayGee Sorry, the project was still private. Please try it again! It's a cmake project, but you can also just copy-paste the single main.cpp into your own project. 🙂

Do we have any output from the various perf tools to peruse?

I'm not entirely sure I'm following... If you're asking for a tool with which the perf. impact can be seen then Visual Studio's built-in Performance Profiler is often sufficient.

[EmJayGee]

Thanks, I'll grab it now.

I'm having enough problems getting things building and running, I was hoping you had saved graphs or captured ETLs or whatever the modern equivalent are that I could peruse.

It seems that if I exit VS, delete bin and obj, restart VS, build and then launch, things maybe work so perhaps I'm on a decent path.

[EmJayGee]

I finally got to see generated code and I'm surprised to find that in retail builds, the generated equality comparison is done piecemeal and that the operator== invocation is not inlined. Perhaps it's not inlined because it's so big so let me see what I can do to fix the former.

Here's the invocation...

0:006> u
OpenConsole!operator!=+0x9 [C:\Users\micha\source\repos\terminal\src\renderer\base\renderer.cpp @ 828] [inlined in OpenConsole!Microsoft::Console::Render::Renderer::_PaintBufferOutputHelper+0x3d7 [C:\Users\micha\source\repos\terminal\src\renderer\base\renderer.cpp @ 828]]:
**00007ff7`d896bb57 e8c4e2ffff      call    OpenConsole!operator== (00007ff7`d8969e20)**
00007ff7`d896bb5c 84c0            test    al,al
00007ff7`d896bb5e 7409            je      OpenConsole!Microsoft::Console::Render::Renderer::_PaintBufferOutputHelper+0x3e9 (00007ff7`d896bb69)
00007ff7`d896bb60 4584d2          test    r10b,r10b
00007ff7`d896bb63 0f8460010000    je      OpenConsole!Microsoft::Console::Render::Renderer::_PaintBufferOutputHelper+0x549 (00007ff7`d896bcc9)
00007ff7`d896bb69 f20f11442470    movsd   mmword ptr [rsp+70h],xmm0
00007ff7`d896bb6f 418b431a        mov     eax,dword ptr [r11+1Ah]
00007ff7`d896bb73 89442478        mov     dword ptr [rsp+78h],eax

and here's the operator == code. It's almost like /Od code which is weird but I did pick the retail config in visual studio.

0:006> u 00007ff7`d8969e20
OpenConsole!operator== [C:\Users\micha\source\repos\terminal\src\buffer\out\TextAttribute.hpp @ 188]:
00007ff7`d8969e20 0fb702          movzx   eax,word ptr [rdx]
00007ff7`d8969e23 663901          cmp     word ptr [rcx],ax
00007ff7`d8969e26 755e            jne     OpenConsole!operator==+0x66 (00007ff7`d8969e86)
00007ff7`d8969e28 0fb64207        movzx   eax,byte ptr [rdx+7]
00007ff7`d8969e2c 384107          cmp     byte ptr [rcx+7],al
00007ff7`d8969e2f 7555            jne     OpenConsole!operator==+0x66 (00007ff7`d8969e86)
00007ff7`d8969e31 0fb64204        movzx   eax,byte ptr [rdx+4]
00007ff7`d8969e35 384104          cmp     byte ptr [rcx+4],al
0:006> u
OpenConsole!operator==+0x10 [C:\Users\micha\source\repos\terminal\src\buffer\out\TextAttribute.hpp @ 189] [inlined in OpenConsole!operator==+0x18 [C:\Users\micha\source\repos\terminal\src\buffer\out\TextAttribute.hpp @ 189]]:
00007ff7`d8969e38 754c            jne     OpenConsole!operator==+0x66 (00007ff7`d8969e86)
00007ff7`d8969e3a 0fb64205        movzx   eax,byte ptr [rdx+5]
00007ff7`d8969e3e 384105          cmp     byte ptr [rcx+5],al
00007ff7`d8969e41 7543            jne     OpenConsole!operator==+0x66 (00007ff7`d8969e86)
00007ff7`d8969e43 0fb64206        movzx   eax,byte ptr [rdx+6]
00007ff7`d8969e47 384106          cmp     byte ptr [rcx+6],al
00007ff7`d8969e4a 753a            jne     OpenConsole!operator==+0x66 (00007ff7`d8969e86)
00007ff7`d8969e4c 0fb6420b        movzx   eax,byte ptr [rdx+0Bh]
0:006> u
OpenConsole!operator==+0x4 [C:\Users\micha\source\repos\terminal\src\buffer\out\TextAttribute.hpp @ 189] [inlined in OpenConsole!operator==+0x30 [C:\Users\micha\source\repos\terminal\src\buffer\out\TextAttribute.hpp @ 189]]:
00007ff7`d8969e50 38410b          cmp     byte ptr [rcx+0Bh],al
00007ff7`d8969e53 7531            jne     OpenConsole!operator==+0x66 (00007ff7`d8969e86)
00007ff7`d8969e55 0fb64208        movzx   eax,byte ptr [rdx+8]
00007ff7`d8969e59 384108          cmp     byte ptr [rcx+8],al
00007ff7`d8969e5c 7528            jne     OpenConsole!operator==+0x66 (00007ff7`d8969e86)
00007ff7`d8969e5e 0fb64209        movzx   eax,byte ptr [rdx+9]
00007ff7`d8969e62 384109          cmp     byte ptr [rcx+9],al
00007ff7`d8969e65 751f            jne     OpenConsole!operator==+0x66 (00007ff7`d8969e86)
0:006> u
OpenConsole!operator==+0x1b [C:\Users\micha\source\repos\terminal\src\buffer\out\TextAttribute.hpp @ 189] [inlined in OpenConsole!operator==+0x47 [C:\Users\micha\source\repos\terminal\src\buffer\out\TextAttribute.hpp @ 189]]:
00007ff7`d8969e67 0fb6420a        movzx   eax,byte ptr [rdx+0Ah]
00007ff7`d8969e6b 38410a          cmp     byte ptr [rcx+0Ah],al
00007ff7`d8969e6e 7516            jne     OpenConsole!operator==+0x66 (00007ff7`d8969e86)
00007ff7`d8969e70 0fb6420c        movzx   eax,byte ptr [rdx+0Ch]
00007ff7`d8969e74 38410c          cmp     byte ptr [rcx+0Ch],al
00007ff7`d8969e77 750d            jne     OpenConsole!operator==+0x66 (00007ff7`d8969e86)
00007ff7`d8969e79 0fb74202        movzx   eax,word ptr [rdx+2]
00007ff7`d8969e7d 66394102        cmp     word ptr [rcx+2],ax
0:006> u
OpenConsole!operator==+0x61 [C:\Users\micha\source\repos\terminal\src\buffer\out\TextAttribute.hpp @ 189]:
00007ff7`d8969e81 7503            jne     OpenConsole!operator==+0x66 (00007ff7`d8969e86)
00007ff7`d8969e83 b001            mov     al,1
00007ff7`d8969e85 c3              ret

[EmJayGee]

OK so just reordering the comparisons gets a much saner operator== implementation. I think that should be step 1 and possibly step omega depending on measurements.

0:000> u 00000001`4001bc67
OpenConsole!operator== [C:\Users\micha\source\repos\terminal\src\renderer\base\renderer.cpp @ 833] [inlined in OpenConsole!Microsoft::Console::Render::Renderer::_PaintBufferOutputHelper+0x4e7 [C:\Users\micha\source\repos\terminal\src\renderer\base\renderer.cpp @ 833]]:
00000001`4001bc67 0fb644245b      movzx   eax,byte ptr [rsp+5Bh]
00000001`4001bc6c 3844247b        cmp     byte ptr [rsp+7Bh],al
00000001`4001bc70 0f85ac010000    jne     OpenConsole!Microsoft::Console::Render::Renderer::_PaintBufferOutputHelper+0x6a2 (00000001`4001be22)
00000001`4001bc76 0fb6442458      movzx   eax,byte ptr [rsp+58h]
00000001`4001bc7b 38442478        cmp     byte ptr [rsp+78h],al
00000001`4001bc7f 0f859d010000    jne     OpenConsole!Microsoft::Console::Render::Renderer::_PaintBufferOutputHelper+0x6a2 (00000001`4001be22)
00000001`4001bc85 0fb6442459      movzx   eax,byte ptr [rsp+59h]
00000001`4001bc8a 38442479        cmp     byte ptr [rsp+79h],al
0:000> u 00000001`4001baeb

[EmJayGee]

... and the memcmp() technique would still have to be done carefully since there is one byte of padding on the end of the struct since the ExtendedAttributes is a BYTE-derived enum but the size of the overall class is 14.

If possible I would just make ExtendedAttributes a WORD-derived enum but that'll probably break too many other things.

I'm prepping a separate PR with just this one change in it that we might agree with since it's super low impact and then we can figure out what else we might want to do.

[zadjii-msft]

Okay so I maybe lost the track here a bit between this thread & #12866 - can someone give me a TL;DR? Do we still want this one, or nah?

[lhecker]

Do we still want this one, or nah?

This is exactly the way til::rect/size/point - 3 other POD types - are being compared. I'd merge this.

[carlos-zamora]

shipit

[carlos-zamora]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[carlos-zamora]

@msftbot merge this in 15 minutes

[ghost]

Hello @carlos-zamora!

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 won't merge this pull request until after the UTC date Thu, 05 May 2022 17:16:29 GMT, which is in 15 minutes

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

[ghost]

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

Handy links:

• Release Notes
• Store Download