Don't use memcmp() to compare TextAttributes, instead just reorder the data member comparisons

[EmJayGee]

This PR is a very small non-semantic change to change the code for comparing TextAttribute instances so that the members are compared in their "physical" order rather than in whatever order the original author chose.

The effect of this is to generate significantly better code for the comparisons. The out-of-order comparisons forced the compiler to load the first cache line, cherry pick the first word of it, then load the second cache line, cherry pick a part of it, and then proceed further along.

This led to another contributor being led to want to write code that used memcmp() to perform the comparison which is usually a sign that things have gone very very wrong. Fortunately this appears to be a simpler solution which gets the vc++ compiler at least to generate the right code.

I have not tried other compilers, only the 14.31.31103 toolkit.

There are other issues in PR #10566 that I'll stay away from but this is simple and straightforward. I also added a compile-time assertion in the header and some comments. I assume the comments are goodness; the static assertion may be a problem on other platforms, I cannot verify.

PR Checklist

• Closes #xxx
• 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

Sorry I don't have anything more detailed than the above plus the code.

I looked at the generated code under cdb before making the fix and then after.

[github-actions]

@check-spelling-bot Report

Unrecognized words, please review:

• numner
• offsertof

Previously acknowledged words that are now absent

azurewebsites cxcy DCompile debolden deconstructed devicefamily GETKEYSTATE guardxfg LLVM LPCHARSETINFO MAPVIRTUALKEY MSDL ned NOWAIT pgorepro pgort PGU redistributable Timeline timelines unintense UWA UWAs VKKEYSCAN WResult xfg

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:EmJayGee/terminal.git repository
on the textattributeopequal branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/10b9044120785735adda548a951329557116a6ab.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/terminal/issues/comments/1093142655" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

• ... misspelled, then please correct them instead of using the command.
• ... names, please add them to .github/actions/spelling/allow/names.txt.
• ... APIs, you can add them to a file in .github/actions/spelling/allow/.
• ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
• ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to a patterns/{file}.txt.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

binary-ish string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

[EmJayGee]

OK I have no idea how to fix this. I have made the spelling error fixes in my branch in my fork, but I don't know where to go from here. I will see if I can keep moving ahead.

[EmJayGee]

aha all I had to do is check in to the branch and push to the fork. I guess that's part of why I couldn't delete the branch in the last issue.

[EmJayGee]

I looked at the generated code again and it's not as good as I thought. I will take another crack at it. I'm surprised at how poor it is. I'd ask the compiler team if it were prohibited from writing the optimizations I would expect here if I was still there but I'm not so. instead I'm led to try writing something that violates the "const refs can actually be passed by value" detente but if a 14 byte struct is ever actually passed by value, I'll find a hat and eat it. I guess when we have 128 bit processors.

[DHowett]

I guess when we have 128 bit processors.

Amen.

Thanks for looking into this as much as you have been! 😄

[EmJayGee]

OK so this code generates pretty much what is desirable. If this is a really hot path, it's possible that a branchless solution would be faster if this is as hot as the other PR author suggested but I think we might be hitting diminishing returns for esoterica. (I do love esoterica but it should generally be in strange little places, not randomly strewn across the code base.)

I will put together an issue and a PR for this new update for people to ruminate upon. It's not as elegant as the original "just swap the order" solution.

I can't figure out where the compiler optimization settings are set for the project. Is it in visual studio or do I need to find some XML file or something?

inline bool operator==(const TextAttribute& a, const TextAttribute& b) noexcept
{
    const char* const left = reinterpret_cast<const char*>(&a);
    const char* const right = reinterpret_cast<const char*>(&b);

    const uint64_t left1 = *reinterpret_cast<const uint64_t*>(left);
    const uint64_t right1 = *reinterpret_cast<const uint64_t*>(right);
    if (left1 != right1)
    {
        return false;
    }

    const uint32_t left2 = *reinterpret_cast<const uint32_t*>(left + sizeof(uint64_t));
    const uint32_t right2 = *reinterpret_cast<const uint32_t*>(right + sizeof(uint64_t));
    if (left2 != right2)
    {
        return false;
    }

    const uint8_t left3 = *reinterpret_cast<const uint8_t*>(left + sizeof(uint64_t) + sizeof(uint32_t));
    const uint8_t right3 = *reinterpret_cast<const uint8_t*>(right + sizeof(uint64_t) + sizeof(uint32_t));
    return left3 == right3;

static_assert((offsetof(TextAttribute, _extendedAttrs) + sizeof(TextAttribute::_extendedAttrs)) == (sizeof(uint64_t) + sizeof(uint32_t) + sizeof(uint8_t)));
}

[EmJayGee]

Notably, the above code is actually inlined which might have broader positive impact.

[DHowett]

Funny enough, if you do write the memcmp version of the comparison operator it compiles down to code that looks nearly identical (admittedly, in assembly) to what you ended up writing.

At the end of the day, that code doesn't seem too different from calling memcmp ourselves -- down to the risks posed by both padding and alignment -- but seemingly at greater risk, right?