Remove reflection in ValueType.Equals/GetHashCode #5436

MichalStrehovsky · 2018-02-23T13:36:07Z

Alternate implementation to #5226 that has a lot less complexity, but it's a bit more costly from size on disk perspective.

The implementation relies on a new virtual method on System.ValueType that provides information about fields on a type. An override of this method is injected by the compiler when needed.

This implementation is different from what Project N does (where we inject actual overrides of both methods). The CoreRT implementation is a bit more space-saving because there is only one method that does fewer things.

This ends up being a 0.9% regression in size of a hello world app, but we do get some correctness with it and a potential to get the reflection stack completely out of the base hello world image (that one will be a huge win). We can get some of the regression back by:

Getting RyuJIT to generate more efficient code for the offset calculation (dotnet/coreclr#16527)
Making fewer things reflectable. We're currently generating the data for e.g. OSVERSIONINFO because it's used as a parameter in a method that is reflectable, and therefore the parameter gets boxed, and therefore it needs this data. It also drags in a fixed buffer internal type because it's a valuetype field on OSVERSIONINFO. This alone costs us 100+ bytes.

MichalStrehovsky · 2018-02-23T13:40:31Z

@mikedn this is where dotnet/coreclr#16527 would help a bit

morganbr · 2018-02-24T06:04:01Z

Do you have any performance measurements for this? The reflection version on ProjectN has prompted some customer complaints when they hit it.

morganbr · 2018-02-24T06:12:24Z

src/System.Private.CoreLib/src/System/ValueType.cs

+
+                // Compare the memory
+                int valueTypeSize = (int)this.EETypePtr.ValueTypeSize;
+                for (int i = 0; i < valueTypeSize; i++)


Is this really the efficient way to do this? I thought a lot of work went into making Span comparison fast

It is not available it CoreLib yet. It will be after dotnet/coreclr#16521.

morganbr · 2018-02-24T06:13:59Z

src/System.Private.CoreLib/src/System/ValueType.cs

+                    int fieldOffset = __GetFieldHelper(i, out EETypePtr fieldType);
+
+                    // Fetch the value of the field on both types
+                    object thisField = RuntimeImports.RhBoxAny(ref Unsafe.Add(ref thisRawData, fieldOffset), fieldType);


Is working on a boxed copy correct? I'd think you could have a self-modifying equals/gethashcode method (yes, I'm entirely clear on how insane that sounds)

It is not quite right, but it matched what CoreCLR and the existing ProjectN reflection-based implementation does for Equals. It is broken in so many different ways...

morganbr · 2018-02-24T06:16:01Z

src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs

+
+            // TODO: what we're shooting for is overlapping fields
+            // or gaps between fields
+            if (type.IsExplicitLayout || type.GetClassLayout().Size != 0)


What happens for padding from alignment? For example:

struct MyStruct { byte b; // byte 0 // 3-7 bytes of padding IntPtr ptr; // byte 4 or 8 }

We wouldn't want to do byte comparisons with padding since it's not guaranteed to be set

Fixed the TODO.

morganbr · 2018-02-24T06:16:46Z

src/System.Private.CoreLib/src/System/ValueType.cs

@@ -26,6 +31,7 @@ public override String ToString()
            return this.GetType().ToString();
        }

+#if PROJECTN


Should/could ProjectN adopt this?

We would just have to update the IL2IL transform that does this. The long term plan is to get rid of transforms, so I would kind of prefer this was another of those things that will be naturally picked up when we do the next step of Project X (get rid of STS type system and host C2 from the CoreRT compiler driver directly).

morganbr · 2018-02-24T06:35:22Z

src/System.Private.CoreLib/src/System/ValueType.cs

+                }
+                else if (fieldType.IsPrimitive)
+                {
+                    hashCode = FastGetValueTypeHashCodeHelper(fieldType, ref fieldData);


I think you've correctly ported this from CoreCLR, but it appears to be yet another bug (not every primitive has a hashcode equal to its value)

Also, I've confirmed that structs smaller than 4 bytes all have the same hash code on CoreCLR because of this and the fact that FastGetValueTypeHashCodeHelper . 😦

https://github.com/dotnet/coreclr/issues/16427

morganbr · 2018-02-24T06:36:19Z

src/System.Private.CoreLib/src/System/ValueType.cs

+
+                Debug.Assert(!fieldType.IsPointer);
+
+                if (fieldType.CorElementType == RuntimeImports.RhCorElementType.ELEMENT_TYPE_R4)


While this looks like a good idea, CoreCLR just pushes this down the XOR path I complained about below

This was supposed to be fixed by dotnet/coreclr#13164. Opened: https://github.com/dotnet/coreclr/issues/16545

morganbr · 2018-02-24T06:38:44Z

src/System.Private.CoreLib/src/System/ValueType.cs

+                    // of __GetFieldHelper, decodes the unboxing stub pointed to by the slot to the real target
+                    // (we already have that part), and calls the entrypoint that expects a byref `this`, and use the
+                    // data to decide between calling fast or regular hashcode helper.
+                    object fieldValue = RuntimeImports.RhBox(fieldType, ref fieldData);


Same boxing issue

The current CoreCLR implementation won't actually box and call GetHashCode for valuetype fields. It will always take xor paths for them (ignoring any GetHashCode overrides that the valuetypes may have).

It will always take xor paths for them

It won't take the XOR path if the nested struct contains GC pointers or floating-point fields. Without building the extra feature the comment is talking about, we can't avoid the box. I'll do some factoring to avoid calling the actual GetHashCode and simulate what CLR would do.

morganbr · 2018-02-24T06:40:32Z

src/System.Private.CoreLib/src/System/ValueType.cs

+                }
+                else
+                {
+                    object fieldValue = Unsafe.Read<object>(ref fieldData);


Is the read guaranteed to be GC safe?

It's an atomic read from a GC tracked interior pointer, so yes.

MichalStrehovsky · 2018-02-26T08:44:28Z

Do you have any performance measurements for this? The reflection version on ProjectN has prompted some customer complaints when they hit it.

Intuitively, it looked faster than the CLR, but it does sound like a good idea to collect some numbers for it:

Test program

static void Main(string[] args)
{
    o1.Equals(o2);
    o1.GetHashCode();

    Stopwatch sw = Stopwatch.StartNew();

    for (int i = 0; i < 1000000; i++)
    {
        o1.Equals(o2);
    }

    Console.WriteLine(sw.ElapsedMilliseconds);

    sw.Restart();

    for (int i = 0; i < 1000000; i++)
    {
        o1.GetHashCode();
    }

    Console.WriteLine(sw.ElapsedMilliseconds);
}

First definition of `Foo`

static object o1 = new Foo();
static object o2 = new Foo { O = new object() };

struct Foo
{
    public int X;
    public int Y;
    public object O;
}

	Run 1	Run 2	Run 3
CLR	384ms/49ms	388ms/49ms	395ms/50ms
CoreRT new	149ms/28ms	149ms/30ms	146ms/28ms
CoreRT old	400ms/960ms	399ms/961ms	395ms/952ms

Second definition of `Foo`

static object o1 = new Foo();
static object o2 = new Foo { Z = 123 };

struct Foo
{
    public int X;
    public int Y;
    public int Z;
}

	Run 1	Run 2	Run 3
CLR	23ms/31ms	23ms/32ms	23ms/31ms
CoreRT new	18ms/15ms	18ms/15ms	18ms/15ms
CoreRT old	468ms/948ms	527ms/961ms	492ms/965ms

MichalStrehovsky · 2018-02-26T08:52:41Z

Looking at the reflection-based ("CoreRT old") GetHashCode slowness, it seems like some of it could be mitigated by not doing the sort by name of the field.

corert/src/System.Private.Reflection.Execution/src/Internal/Reflection/Execution/ReflectionExecutionDomainCallbacksImplementation.cs

Lines 155 to 166 in 91fde46

    
           // The algorithm is to use the hash of the first non-null instance field sorted by name. 
        
           List<FieldInfo> sortedFilteredFields = new List<FieldInfo>(); 
        
           foreach (FieldInfo field in valueType.GetType().GetTypeInfo().DeclaredFields) 
        
           { 
        
               if (field.IsStatic) 
        
               { 
        
                   continue; 
        
               } 
        
               sortedFilteredFields.Add(field); 
        
           } 
        
           sortedFilteredFields.Sort(FieldInfoNameComparer.Instance);

@morganbr Where is the lexicographical sort coming from? Testing this on the desktop CLR, it seems to be taking the first field in metadata order, not lexicographical order.

I would still not want to run that code because it will still be slow, but maybe until Project N is able to adopt the new scheme, we should look at this part?

The implementation relies on a new virtual method on `System.ValueType` that provides information about fields on a type. An override of this method is injected by the compiler when needed. This implementation is different from what Project N does (where we inject actual overrides of both methods). The CoreRT implementation is a bit more space-saving because there is only one method that does fewer things. This ends up being a 0.9% regression in size of a hello world app, but we do get some correctness with it and a potential to get the reflection stack completely out of the base hello world image (that one will be a huge win). We can get some of the regression back by: * Getting RyuJIT to generate more efficient code for the offset calculation (dotnet/coreclr#16527) * Making fewer things reflectable. We're currently generating the data for e.g. OSVERSIONINFO because it's used as a parameter in a method that is reflectable, and therefore the parameter gets boxed, and therefore it needs this data. It also drags in a fixed buffer internal type because it's a valuetype field on OSVERSIONINFO. This alone costs us 100+ bytes.

MichalStrehovsky added 5 commits February 22, 2018 11:00

WIP

a074418

WIP

432a1d3

WIP

044df31

Not worth it

2812680

Fixes

0790b8f

MichalStrehovsky mentioned this pull request Feb 23, 2018

[WIP] Remove reflection in ValueType.Equals/GetHashCode #5226

Closed

jkotas approved these changes Feb 24, 2018

View reviewed changes

morganbr reviewed Feb 24, 2018

View reviewed changes

MichalStrehovsky added 2 commits February 26, 2018 10:21

Update not to call GetHashCode

0f0e0f9

Compute gaps and overlapping fields

554d73a

MichalStrehovsky merged commit 32758a8 into dotnet:master Feb 27, 2018

MichalStrehovsky deleted the codeBasedEquals branch February 27, 2018 10:12

MichalStrehovsky mentioned this pull request Feb 27, 2018

Base class library size on disk footprint workitems #5013

Open

6 tasks

MichalStrehovsky mentioned this pull request Feb 2, 2021

Limit the overhead of ValueType.Equals/GetHashCode support dotnet/runtimelab#624

Merged

MichalStrehovsky mentioned this pull request May 25, 2022

Speed up ValueType.Equals dotnet/runtime#69768

Merged

MichalStrehovsky mentioned this pull request Mar 14, 2023

Do not generate Equals/GetHashCode support for async state machines dotnet/runtime#83369

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove reflection in ValueType.Equals/GetHashCode #5436

Remove reflection in ValueType.Equals/GetHashCode #5436

MichalStrehovsky commented Feb 23, 2018

MichalStrehovsky commented Feb 23, 2018

morganbr commented Feb 24, 2018

morganbr Feb 24, 2018

jkotas Feb 24, 2018

morganbr Feb 24, 2018

jkotas Feb 24, 2018

morganbr Feb 24, 2018

MichalStrehovsky Feb 26, 2018

morganbr Feb 24, 2018

MichalStrehovsky Feb 26, 2018

morganbr Feb 24, 2018

morganbr Feb 24, 2018

jkotas Feb 24, 2018

morganbr Feb 24, 2018

jkotas Feb 24, 2018

morganbr Feb 24, 2018

jkotas Feb 24, 2018

MichalStrehovsky Feb 26, 2018

morganbr Feb 24, 2018

MichalStrehovsky Feb 26, 2018

MichalStrehovsky commented Feb 26, 2018

MichalStrehovsky commented Feb 26, 2018


		Debug.Assert(!fieldType.IsPointer);

		if (fieldType.CorElementType == RuntimeImports.RhCorElementType.ELEMENT_TYPE_R4)

Remove reflection in ValueType.Equals/GetHashCode #5436

Remove reflection in ValueType.Equals/GetHashCode #5436

Conversation

MichalStrehovsky commented Feb 23, 2018

MichalStrehovsky commented Feb 23, 2018

morganbr commented Feb 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalStrehovsky commented Feb 26, 2018

Test program

First definition of Foo

Second definition of Foo

MichalStrehovsky commented Feb 26, 2018

First definition of `Foo`

Second definition of `Foo`