Simplify Guid comparisons

[pentp]

Remove redundant parts and streamline the rest.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR refactors Guid comparison logic in System.Private.CoreLib to reduce repetitive per-field comparisons by leveraging built-in CompareTo implementations and collapsing the final 8-byte comparison into a single 64-bit Big Endian read.

Changes:

• Simplified Guid.CompareTo(Guid) by using unsigned CompareTo on _a, _b, _c and a single ulong comparison for the trailing bytes.
• Simplified relational operators by comparing _a, _b, _c as unsigned and comparing the remaining 8 bytes via a helper.
• Reduced duplicated logic in > / >= by delegating to < / <= with swapped operands.

---

You can also share your feedback on Copilot code review. Take the survey.

[EgorBo]

This PR effecitvely introduces a new unsafe code where previously it was 100% safe - it is something we try to avoid. MemoryMarshal.CreateReadOnlySpan will soon require unsafe context.

[pentp]

This PR effecitvely introduces a new unsafe code where previously it was 100% safe - it is something we try to avoid. MemoryMarshal.CreateReadOnlySpan will soon require unsafe context.

Avoiding unsafe in general is good, but in this case it's one of the safest ways to use MemoryMarshal.CreateReadOnlySpan and avoids 200 lines of mindless code.

[EgorBo]

This PR effecitvely introduces a new unsafe code where previously it was 100% safe - it is something we try to avoid. MemoryMarshal.CreateReadOnlySpan will soon require unsafe context.

Avoiding unsafe in general is good, but in this case it's one of the safest ways to use MemoryMarshal.CreateReadOnlySpan and avoids 200 lines of mindless code.

It's not just good, it's our priorty to remove unsafe code. I believe Guid can be made 100% safe (with a few trick from the JIT side), so we'd prefer some safe way to reduce lines of code (maybe just compact them?)

[EgorBo]

@pentp how about:

    public int CompareTo(Guid other)
    {
        int c;
        return (c = ((uint)_a).CompareTo((uint)other._a)) != 0 ? c :
            (c = ((uint)_b).CompareTo((uint)other._b)) != 0 ? c :
            (c = ((uint)_c).CompareTo((uint)other._c)) != 0 ? c :
            (c = _d.CompareTo(other._d)) != 0 ? c :
            (c = _e.CompareTo(other._e)) != 0 ? c :
            (c = _f.CompareTo(other._f)) != 0 ? c :
            (c = _g.CompareTo(other._g)) != 0 ? c :
            (c = _h.CompareTo(other._h)) != 0 ? c :
            (c = _i.CompareTo(other._i)) != 0 ? c :
            (c = _j.CompareTo(other._j)) != 0 ? c :
            _k.CompareTo(other._k);
    }

it also seems to generate better codegen

[pentp]

it also seems to generate better codegen

How is this better: https://www.diffchecker.com/4vOXIxTU/

Avoiding unsafe at all costs will cause the same issues as avoiding goto - convoluted code that's more inefficient and often harder to understand with no other benefit than following "best practices".

[tannergooding]

The code today is not convoluted and the “more expensive” is around 1ns difference

I don’t believe the “increase here” is worth the unsafeness and making the code smaller is not “better” either, it just makes the code that much harder to follow and understand

This is highly unlikely to be a bottleneck and if it was, it is trivial to provide your own custom comparator

———-

if we did care enough, the JIT could potentially recognize the pattern of sequential byte or ushort compares or we could use a pattern of building the uint/ulong it could recognize such that it could itself emit the ReadBigEndian equivalent. But I’m not sure the cost is worth it there either

[tannergooding]

Would appreciate weigh-in from someone else as well, like @stephentoub.

I personally find this new code to be significantly less readable and understandable. It's also seemingly unnecessary churn as it remains within the nanosecond range of improvements, so is much more likely to be meaningless in the context of cache misses and branch mispredicts that real world code will likely hit here.

The original code was "long", but it was also incredibly straightforward, easy to follow, and trivial to understand. It was also efficient, as is seen by the microscopic improvements the new code provides.

[stephentoub]

Is this just a stylistic change? Or if not, what aspect of the change is contributing to a perf improvement?

[tannergooding]

It gives a single return and inverts the default branch path, improving codegen slightly, but remains within the nanosecond level so could be argued is purely stylistic

-- the default IL and codegen emitted, barring PGO stating otherwise, is if (cond) { predicted } else { unpredicted } vs cond ? unpredicted : predicted

[EgorBo]

I don't think perf matters here, but being able to remove -300 LOC from Guid IMO is nice +16 -294

[tannergooding]

I disagree when it makes the code less readable. Less lines of code is not strictly better, particularly when most of those lines are braces and new lines separating logical blocks.

We could still simplify a lot of this by deferring other comparisons to CompareTo, which doesn't hurt readability; but I strongly think the removal of the braces and clear linear flow to instead have a highly compressed block of nested ternary expressions (with nested side effecting assignments) is so much worse.

[pentp]

While in general I dislike overly verbose code, that was not the main point of this PR. After some more thinking I came up with a new variant that should be both readable and compact - what do you think?

[tannergooding]

The new variant is still introducing new unsafe code and adding complexity to a method that is already very efficient and trivial to understand.

[pentp]

The existing method is definitely not efficient (the apparent simplicity of it is misleading).
I did some more cleanup, the latest version should be even simpler. And BitCast isn't memory unsafe.

[tannergooding]

The existing method is definitely not efficient (the apparent simplicity of it is misleading).

The perf numbers show a difference in terms of around 1 nanosecond. That is incredibly efficient and well within the noise of the system.

And BitCast isn't memory unsafe.

It is explicitly memory unsafe and will be [RequiresUnsafe]. Not only does it allow something like struct S1 { object o; } to struct S2 { string o; }, but it also allows you to create illegal bit representations for types, such as decimal.

[tannergooding]

The perf numbers show a difference in terms of around 1 nanosecond. That is incredibly efficient and well within the noise of the system.

Put another way, the size of the disassembly doesn't really matter. Size does not correlate to performance. There are many trivial examples where longer instruction sequences are faster and where shorter ones are slower. There are also many examples where the differences do not matter for real world perf.

It is explicitly memory unsafe and will be [RequiresUnsafe]

Now that being said, Guid allows "any bit pattern" and so there 'could' theoretically be an API like BitConverter.GuidToInt128Bits, mirroring BitConverter.SingleToInt32Bits and friends. This would internally use Unsafe.BitCast and abstract away the unsafety.

But that's a discussion for whether or not its actually worthwhile and I'd think we need an API proposal to discuss that. The nanosecond differences here do not justify the change itself.

[Copilot]

Pull request overview

This PR refactors Guid comparison logic to reduce duplicated field-by-field comparison code by centralizing ordering in Guid.CompareTo(Guid) and delegating relational operators to it.

Changes:

• Reworked Guid.CompareTo(Guid) to use a UInt128 bit-cast path for endianness-sensitive comparison.
• Simplified Guid relational operators (<, <=, >, >=) to delegate to CompareTo.
• Changed UInt128 backing fields’ accessibility to enable Guid to read the upper 64-bits of the bit-cast value.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/libraries/System.Private.CoreLib/src/System/UInt128.cs	Makes _upper/_lower internal to support Guid’s new comparison implementation.
src/libraries/System.Private.CoreLib/src/System/Guid.cs	Refactors CompareTo(Guid) and routes relational operators through it.