Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve EqualityComparer for NativeAOT #83054

Merged
merged 2 commits into from Mar 7, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Mar 7, 2023

Closes #81592

static bool Test(int a, int b) =>
    EqualityComparer<int>.Default.Equals(a, b);

Old codegen on NativeAOT:

; Method Program:Bar[int](int,int):bool
       57                   push     rdi
       56                   push     rsi
       4883EC28             sub      rsp, 40
       8BF1                 mov      esi, ecx
       8BFA                 mov      edi, edx
       488B0500000000       mov      rax, qword ptr [(reloc 0x4000000000425988)]
       4883780800           cmp      gword ptr [rax+08H], 0
       7505                 jne      SHORT G_M49161_IG04
       E800000000           call     System.Collections.Generic.EqualityComparer`1[int]:Create()
G_M49161_IG04:              
       33C0                 xor      eax, eax
       3BF7                 cmp      esi, edi
       0F94C0               sete     al
       4883C428             add      rsp, 40
       5E                   pop      rsi
       5F                   pop      rdi
       C3                   ret      
; Total bytes of code: 43

New codegen:

; Method Program:Bar[int](int,int):bool
       33C0                 xor      eax, eax
       3BCA                 cmp      ecx, edx
       0F94C0               sete     al
       C3                   ret      
; Total bytes of code: 8

cc @dotnet/ilc-contrib @MichalStrehovsky

@ghost
Copy link

ghost commented Mar 7, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Closes #81592

static bool Test(int a, int b) =>
    EqualityComparer<int>.Default.Equals(a, b);

Old codegen on NativeAOT:

; Method Program:Bar[int](int,int):bool
       57                   push     rdi
       56                   push     rsi
       4883EC28             sub      rsp, 40
       8BF1                 mov      esi, ecx
       8BFA                 mov      edi, edx
       488B0500000000       mov      rax, qword ptr [(reloc 0x4000000000425988)]
       4883780800           cmp      gword ptr [rax+08H], 0
       7505                 jne      SHORT G_M49161_IG04
       E800000000           call     System.Collections.Generic.EqualityComparer`1[int]:Create()
G_M49161_IG04:              
       33C0                 xor      eax, eax
       3BF7                 cmp      esi, edi
       0F94C0               sete     al
       4883C428             add      rsp, 40
       5E                   pop      rsi
       5F                   pop      rdi
       C3                   ret      
; Total bytes of code: 43

New codegen:

; Method Program:Bar[int](int,int):bool
       33C0                 xor      eax, eax
       3BCA                 cmp      ecx, edx
       0F94C0               sete     al
       C3                   ret      
; Total bytes of code: 8

cc @dotnet/ilc-contrib @MichalStrehovsky

Author: EgorBo
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@EgorBo
Copy link
Member Author

EgorBo commented Mar 7, 2023

More complicated example involving generics:

using System.Collections.Generic;
using System.Runtime.CompilerServices;

class Program
{
    static void Main()
    {
        Foo(1, 2);
        Foo("1", "2");
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static bool Foo<T>(T a, T b) => Bar(a, b);

    [MethodImpl(MethodImplOptions.NoInlining)]
    static bool Bar<T>(T a, T b) => 
        EqualityComparer<T>.Default.Equals(a, b);
}

codegen diff: https://www.diffchecker.com/MyttN9cW/

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@MichalStrehovsky MichalStrehovsky merged commit 1247d8f into dotnet:main Mar 7, 2023
108 checks passed
MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull request Mar 7, 2023
If a type is preinitialized, we shouldn't need the cctor pointer anymore. This was the only thing keeping around general-purpose comparers logic after @EgorBo's dotnet#83054.
MichalStrehovsky added a commit that referenced this pull request Mar 7, 2023
If a type is preinitialized, we shouldn't need the cctor pointer anymore. This was the only thing keeping around general-purpose comparers logic after @EgorBo's #83054.
@EgorBo EgorBo deleted the naot-equality-comparer-fix branch March 7, 2023 08:37
@eerhardt
Copy link
Member

eerhardt commented Mar 7, 2023

It looks like this caused a ~50KB size on disk regression on the BasicMinimalApi app on linux-x64.

The regression happened in this commit range.

Doing an analysis of the .mstat files before and after, I see most of the size increase in System.Collections.Generic.

image

And then dumping the difference in that namespace, the biggest chunk that stands out:

image

(note there are many more ObjectComparers on the right, these are just a snippet of them. There are also new ObjectEqualityComparer and GenericEqualityComparer instances as well.)

Attached are the mstat files and dumping them to txt.
BasicMinimalApi-regression.zip

@agocke
Copy link
Member

agocke commented Mar 7, 2023

Is it possible that #83063 will bring things back into alignment?

@eerhardt
Copy link
Member

eerhardt commented Mar 7, 2023

Is it possible that #83063 will bring things back into alignment?

Unfortunately, no. Note that both this commit and that commit are in the above commit range. So the 50KB regression is after both changes.

@eerhardt
Copy link
Member

eerhardt commented Mar 7, 2023

Also note that the regression on win-x64 seems to be smaller. More like ~20KB.

@EgorBo
Copy link
Member Author

EgorBo commented Mar 7, 2023

@MichalStrehovsky any ideas what we can do here to avoid type loading in runtime? if not - can we somehow do lazy init only for non-value type instanciations?

@MichalStrehovsky
Copy link
Member

I looked at this in a bit more detail. Previously, comparer types for reference type T were always loaded lazily - we generated the code for "any reference type", and at runtime we would land here where we essentially MakeGenericType the right comparer:

internal static object GetComparer(RuntimeTypeHandle t)
{
RuntimeTypeHandle comparerType;
RuntimeTypeHandle openComparerType = default(RuntimeTypeHandle);
RuntimeTypeHandle comparerTypeArgument = default(RuntimeTypeHandle);
if (RuntimeAugments.IsNullable(t))
{
RuntimeTypeHandle nullableType = RuntimeAugments.GetNullableType(t);
openComparerType = typeof(NullableComparer<>).TypeHandle;
comparerTypeArgument = nullableType;
}
if (EqualityComparerHelpers.IsEnum(t))
{
openComparerType = typeof(EnumComparer<>).TypeHandle;
comparerTypeArgument = t;
}
if (openComparerType.Equals(default(RuntimeTypeHandle)))
{
if (ImplementsIComparable(t))
{
openComparerType = typeof(GenericComparer<>).TypeHandle;
comparerTypeArgument = t;
}
else
{
openComparerType = typeof(ObjectComparer<>).TypeHandle;
comparerTypeArgument = t;
}
}
bool success = RuntimeAugments.TypeLoaderCallbacks.TryGetConstructedGenericTypeForComponents(openComparerType, new RuntimeTypeHandle[] { comparerTypeArgument }, out comparerType);
if (!success)
{
Environment.FailFast("Unable to create comparer");
}
return RuntimeAugments.NewObject(comparerType);
}

With the change in this PR, comparers are preinitialized at compile time (we actually preallocate the object instance).

It is a compromise. Doing things lazily helps size, but impacts startup. For the BasicWebApi sample, we were previously loading 3 comparers at startup and another 4 on first request. So we avoid 7 type loads. The 7 type loads cost about 0.2 ms in startup time in total. So we're basically looking at <0.5% size regression for <0.5% startup improvement.

If it was more than 0.5% I would be more concerned but maybe this is an acceptable tradeoff?

@eerhardt
Copy link
Member

eerhardt commented Mar 8, 2023

but maybe this is an acceptable tradeoff?

Yeah, it sounds like it. I just wanted to raise the size regression here just to see if there was anything we could do. But since this is so small, if there isn't anything trivial/obvious we can just live with it.

@MichalPetryka
Copy link
Contributor

Can this be removed now?

#if !NATIVEAOT
return EqualityComparer<T>.Default.IndexOf(array, value, startIndex, count);
#else
return IndexOfImpl(array, value, startIndex, count);
#endif

@MichalStrehovsky
Copy link
Member

Can this be removed now?

It would likely bring a lot more comparers for no clear benefit.

@dotnet dotnet locked as resolved and limited conversation to collaborators Apr 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NativeAOT] Usages of generic EqualityComparer.Default have cctor init checks for primitives
5 participants