Move unboxing helpers to managed code #109135

davidwrighton · 2024-10-23T00:14:45Z

Move the unboxing helpers to managed code.

Behavior is basically identical except for the Unbox_Nullable paths, which required some investigation to find the fastest implementation. Notably, there interruptibility of managed code makes the copying/zeroing of values more difficult, but with the opportunity/requirement to specialize the codebase came a few micro-optimizations that are somewhat nice. Overall I don't expect anyone to notice the performance changes here, but since my earlier code was about 2X slower than the native implementation, I did feel the need to optimize until everything looked good.

Performance results:

TestName	With PR	Without PR	% Speedup
TestJustAPrimitive	1217.0047	1221.203	0.34%
TestJustObject	1212.6415	1211.6437	-0.08%
TestJust5Primitive	1496.5304	1522.4968	1.74%
TestJust5Object	1461.0507	1488.3328	1.87%
TestJust10Primitive	1473.2814	1493.5238	1.37%
TestJust10Object	3215.6339	2854.6186	-11.23%
TestJustAPrimitiveNullableWithValue	2727.9085	5182.2611	89.97%
TestJustObjectNullableWithValue	3148.9484	5672.2985	80.13%
TestJust5PrimitiveNullableWithValue	5443.9232	7795.6109	43.20%
TestJust5ObjectNullableWithValue	6492.9071	8095.1508	24.68%
TestJust10PrimitiveNullableWithValue	6022.6274	8723.572	44.85%
TestJust10ObjectNullableWithValue	7728.3239	9671.1382	25.14%
TestJustAPrimitiveNullNullable	1786.1337	2230.0932	24.86%
TestJustObjectNullNullable	1675.0683	2326.0395	38.86%
TestJust5PrimitiveNullNullable	2921.9497	3298.4642	12.89%
TestJust5ObjectNullNullable	3389.4043	3615.3131	6.67%
TestJust10PrimitiveNullNullable	3050.809	4054.9683	32.91%
TestJust10ObjectNullNullable	4658.8316	5335.0686	14.52%

Results are very positive, or within the margin of error in this test suite. These results were generated using a small benchmark which mostly targeted measuring the performance of the Unbox_Nullable helper, as it has the most complex and potentially slow code. Generally the impact on that helper is that the performance of the type system portion of the helper is faster, and the performance of code which actually copies the contents of a valuetype is a little better. This isn't quite a fair test of managed vs native performance though, as I took the opportunity to restructure some of the memory on MethodTable so that it could more easily be read in managed code, and that happened to make a fair bit of complex code become simpler and thus faster.

Benchmark code (standalone console app)

using System.Diagnostics;
using System.Runtime.CompilerServices;

namespace UnboxingPerfTests
{
    internal class Program
    {
        public interface IStaticMethod<T>
        {
            public static abstract void Method(ref T param);
        }
        // All tests use GenericStruct<T, object> to force us into canonical code gen to always use the helpers
        struct GenericStruct<T, V> : IStaticMethod<GenericStruct<T, V>> where T: IStaticMethod<T>
        {
            public T Value;

            [MethodImpl(MethodImplOptions.NoInlining)]
            public static void Method(ref GenericStruct<T, V> value)
            {
                T.Method(ref value.Value);
            }
        }

        struct JustAPrimitive : IStaticMethod<JustAPrimitive>
        {
            public int Value;
            public static void Method(ref JustAPrimitive param)
            {
            }
        }

        struct JustObject : IStaticMethod<JustObject>
        {
            public object Value;
            public static void Method(ref JustObject param)
            {
            }
        }

        struct Just10Primitive : IStaticMethod<Just10Primitive>
        {
            public int Value;
            public int Value2;
            public int Value3;
            public int Value4;
            public int Value5;
            public int Value6;
            public int Value7;
            public int Value8;
            public int Value9;
            public int Value10;
            public static void Method(ref Just10Primitive param)
            {
            }
        }

        struct Just10Object : IStaticMethod<Just10Object>
        {
            public object Value;
            public object Value2;
            public object Value3;
            public object Value4;
            public object Value5;
            public object Value6;
            public object Value7;
            public object Value8;
            public object Value9;
            public object Value10;

            public static void Method(ref Just10Object param)
            {
            }
        }

        struct Just5Primitive : IStaticMethod<Just5Primitive>
        {
            public int Value;
            public int Value2;
            public int Value3;
            public int Value4;
            public int Value5;
            public static void Method(ref Just5Primitive param)
            {
            }
        }

        struct Just5Object : IStaticMethod<Just5Object>
        {
            public object Value;
            public object Value2;
            public object Value3;
            public object Value4;
            public object Value5;

            public static void Method(ref Just5Object param)
            {
            }
        }


        public static void Unbox_any_Test<T>(object[] o) where T:IStaticMethod<T>
        {
            for (int i = 0; i < 1_000_000; i++)
            {
                T local = (T)o[i%3];
                T.Method(ref local);
            }
        }

        [MethodImpl(MethodImplOptions.NoInlining)]
        public static int IsInst_Test<T>(object?[] o)
        {
            int retVal = 0;
            for (int i = 0; i < 1_000_000; i++)
            {
                if (o[i % 3] is T)
                    retVal++;
            }
            return retVal;
        }

        public static void Unbox_any_TestNullable<T>(object?[] o) where T : struct, IStaticMethod<T>
        {
            for (int i = 0; i < 1_000_000; i++)
            {
                T? local = (T?)o[i % 3];
                if (local.HasValue)
                {
                    T localCopy = local.Value;
                    T.Method(ref localCopy);
                }
            }
        }


        [MethodImpl(MethodImplOptions.AggressiveOptimization)]
        public static void Test(int outerIterationCount, Action a, string testName, Action<string> outputFunc)
        {
            Stopwatch stopwatch = Stopwatch.StartNew();
            for (int i = 0; i < outerIterationCount; i++)
            {
                a();
            }
            stopwatch.Stop();
            outputFunc($"| {testName,38}| {stopwatch.Elapsed.TotalMilliseconds.ToString()}|");
        }

        static void TestJustAPrimitive()
        {
            object[] arr = { new GenericStruct<JustAPrimitive, object>(), new GenericStruct<JustAPrimitive, object>(), new GenericStruct<JustAPrimitive, object>() };
            Unbox_any_Test<GenericStruct<JustAPrimitive, object>>(arr);
        }

        static void TestJustObject()
        {
            object[] arr = { new GenericStruct<JustObject, object>(), new GenericStruct<JustObject, object>(), new GenericStruct<JustObject, object>() };
            Unbox_any_Test<GenericStruct<JustObject, object>>(arr);
        }

        static void TestJust10Primitive()
        {
            object[] arr = { new GenericStruct<Just10Primitive, object>(), new GenericStruct<Just10Primitive, object>(), new GenericStruct<Just10Primitive, object>() };
            Unbox_any_Test<GenericStruct<Just10Primitive, object>>(arr);
        }

        static void TestJust10Object()
        {
            object[] arr = { new GenericStruct<Just10Object, object>(), new GenericStruct<Just10Object, object>(), new GenericStruct<Just10Object, object>() };
            Unbox_any_Test<GenericStruct<Just10Object, object>>(arr);
        }

        static void TestJust5Primitive()
        {
            object[] arr = { new GenericStruct<Just5Primitive, object>(), new GenericStruct<Just5Primitive, object>(), new GenericStruct<Just5Primitive, object>() };
            Unbox_any_Test<GenericStruct<Just5Primitive, object>>(arr);
        }

        static void TestJust5Object()
        {
            object[] arr = { new GenericStruct<Just5Object, object>(), new GenericStruct<Just5Object, object>(), new GenericStruct<Just5Object, object>() };
            Unbox_any_Test<GenericStruct<Just5Object, object>>(arr);
        }


        static void TestJustAPrimitiveNullableWithValue()
        {
            object[] arr = { new GenericStruct<JustAPrimitive, object>(), new GenericStruct<JustAPrimitive, object>(), new GenericStruct<JustAPrimitive, object>() };
            Unbox_any_TestNullable<GenericStruct<JustAPrimitive, object>>(arr);
        }

        static void TestJustObjectNullableWithValue()
        {
            object[] arr = { new GenericStruct<JustObject, object>(), new GenericStruct<JustObject, object>(), new GenericStruct<JustObject, object>() };
            Unbox_any_TestNullable<GenericStruct<JustObject, object>>(arr);
        }

        static void TestJust10PrimitiveNullableWithValue()
        {
            object[] arr = { new GenericStruct<Just10Primitive, object>(), new GenericStruct<Just10Primitive, object>(), new GenericStruct<Just10Primitive, object>() };
            Unbox_any_TestNullable<GenericStruct<Just10Primitive, object>>(arr);
        }

        static void TestJust10ObjectNullableWithValue()
        {
            object[] arr = { new GenericStruct<Just10Object, object>(), new GenericStruct<Just10Object, object>(), new GenericStruct<Just10Object, object>() };
            Unbox_any_TestNullable<GenericStruct<Just10Object, object>>(arr);
        }

        static void TestJust5PrimitiveNullableWithValue()
        {
            object[] arr = { new GenericStruct<Just5Primitive, object>(), new GenericStruct<Just5Primitive, object>(), new GenericStruct<Just5Primitive, object>() };
            Unbox_any_TestNullable<GenericStruct<Just5Primitive, object>>(arr);
        }

        static void TestJust5ObjectNullableWithValue()
        {
            object[] arr = { new GenericStruct<Just5Object, object>(), new GenericStruct<Just5Object, object>(), new GenericStruct<Just5Object, object>() };
            Unbox_any_TestNullable<GenericStruct<Just5Object, object>>(arr);
        }


        static void TestJustAPrimitiveNullNullable()
        {
            object?[] arr = { null, null, null };
            Unbox_any_TestNullable<GenericStruct<JustAPrimitive, object>>(arr);
        }

        static void TestJustObjectNullNullable()
        {
            object?[] arr = { null, null, null };
            Unbox_any_TestNullable<GenericStruct<JustObject, object>>(arr);
        }

        static void TestJust10PrimitiveNullNullable()
        {
            object?[] arr = { null, null, null };
            Unbox_any_TestNullable<GenericStruct<Just10Primitive, object>>(arr);
        }

        static void TestJust10ObjectNullNullable()
        {
            object?[] arr = { null, null, null };
            Unbox_any_TestNullable<GenericStruct<Just10Object, object>>(arr);
        }

        static void TestJust5PrimitiveNullNullable()
        {
            object?[] arr = { null, null, null };
            Unbox_any_TestNullable<GenericStruct<Just5Primitive, object>>(arr);
        }

        static void TestJust5ObjectNullNullable()
        {
            object?[] arr = { null, null, null };
            Unbox_any_TestNullable<GenericStruct<Just5Object, object>>(arr);
        }

        static void IsInstTestJustAPrimitive()
        {
            object[] arr = { new GenericStruct<JustAPrimitive, object>(), new GenericStruct<JustAPrimitive, object>(), new GenericStruct<JustAPrimitive, object>() };
            IsInst_Test<GenericStruct<JustAPrimitive, object>>(arr);
        }

        static void IsInstTestJustObject()
        {
            object[] arr = { new GenericStruct<JustObject, object>(), new GenericStruct<JustObject, object>(), new GenericStruct<JustObject, object>() };
            IsInst_Test<GenericStruct<JustObject, object>>(arr);
        }

        static void IsInstTestJust10Primitive()
        {
            object[] arr = { new GenericStruct<Just10Primitive, object>(), new GenericStruct<Just10Primitive, object>(), new GenericStruct<Just10Primitive, object>() };
            IsInst_Test<GenericStruct<Just10Primitive, object>>(arr);
        }

        static void IsInstTestJust10Object()
        {
            object[] arr = { new GenericStruct<Just10Object, object>(), new GenericStruct<Just10Object, object>(), new GenericStruct<Just10Object, object>() };
            IsInst_Test<GenericStruct<Just10Object, object>>(arr);
        }

        static void IsInstTestJust5Primitive()
        {
            object[] arr = { new GenericStruct<Just5Primitive, object>(), new GenericStruct<Just5Primitive, object>(), new GenericStruct<Just5Primitive, object>() };
            IsInst_Test<GenericStruct<Just5Primitive, object>>(arr);
        }

        static void IsInstTestJust5Object()
        {
            object[] arr = { new GenericStruct<Just5Object, object>(), new GenericStruct<Just5Object, object>(), new GenericStruct<Just5Object, object>() };
            IsInst_Test<GenericStruct<Just5Object, object>>(arr);
        }


        static void IsInstTestJustAPrimitiveNullableWithValue()
        {
            object[] arr = { new GenericStruct<JustAPrimitive, object>(), new GenericStruct<JustAPrimitive, object>(), new GenericStruct<JustAPrimitive, object>() };
            IsInst_Test<GenericStruct<JustAPrimitive, object>>(arr);
        }

        static void IsInstTestJustObjectNullableWithValue()
        {
            object[] arr = { new GenericStruct<JustObject, object>(), new GenericStruct<JustObject, object>(), new GenericStruct<JustObject, object>() };
            IsInst_Test<GenericStruct<JustObject, object>>(arr);
        }

        static void IsInstTestJust10PrimitiveNullableWithValue()
        {
            object[] arr = { new GenericStruct<Just10Primitive, object>(), new GenericStruct<Just10Primitive, object>(), new GenericStruct<Just10Primitive, object>() };
            IsInst_Test<GenericStruct<Just10Primitive, object>?>(arr);
        }

        static void IsInstTestJust10ObjectNullableWithValue()
        {
            object[] arr = { new GenericStruct<Just10Object, object>(), new GenericStruct<Just10Object, object>(), new GenericStruct<Just10Object, object>() };
            IsInst_Test<GenericStruct<Just10Object, object>?>(arr);
        }

        static void IsInstTestJust5PrimitiveNullableWithValue()
        {
            object[] arr = { new GenericStruct<Just5Primitive, object>(), new GenericStruct<Just5Primitive, object>(), new GenericStruct<Just5Primitive, object>() };
            IsInst_Test<GenericStruct<Just5Primitive, object>?>(arr);
        }

        static void IsInstTestJust5ObjectNullableWithValue()
        {
            object[] arr = { new GenericStruct<Just5Object, object>(), new GenericStruct<Just5Object, object>(), new GenericStruct<Just5Object, object>() };
            IsInst_Test<GenericStruct<Just5Object, object>?>(arr);
        }


        static void IsInstTestJustAPrimitiveNullNullable()
        {
            object?[] arr = { null, null, null };
            IsInst_Test<GenericStruct<JustAPrimitive, object>?>(arr);
        }

        static void IsInstTestJustObjectNullNullable()
        {
            object?[] arr = { null, null, null };
            IsInst_Test<GenericStruct<JustObject, object>?>(arr);
        }

        static void IsInstTestJust10PrimitiveNullNullable()
        {
            object?[] arr = { null, null, null };
            IsInst_Test<GenericStruct<Just10Primitive, object>?>(arr);
        }

        static void IsInstTestJust10ObjectNullNullable()
        {
            object?[] arr = { null, null, null };
            IsInst_Test<GenericStruct<Just10Object, object>?>(arr);
        }

        static void IsInstTestJust5PrimitiveNullNullable()
        {
            object?[] arr = { null, null, null };
            IsInst_Test<GenericStruct<Just5Primitive, object>?>(arr);
        }

        static void IsInstTestJust5ObjectNullNullable()
        {
            object?[] arr = { null, null, null };
            IsInst_Test<GenericStruct<Just5Object, object>?>(arr);
        }

        enum Foo : int
        {
            Value = 3
        }

        enum Bar : int
        {

        }

        enum Other : int
        {
            Value = 4,
        }

        public static object[] TheArrayForEnumTests = { (int)4, Foo.Value, Other.Value };
        static void IsInstTestEnumVariance()
        {
            int count = 0;
            object[] arr = TheArrayForEnumTests;

            for (int i = 0; i < 1_000_000; i++)
            {
                Bar local = (Bar)arr[i % 3];
                count += (int)local;
            }

            s_x = count;
        }
        public static int s_x;
        static void IsInstTestEnumVarianceNullable()
        {
            int count = 0;
            object?[] arr = TheArrayForEnumTests;

            for (int i = 0; i < 1_000_000; i++)
            {
                Bar? local = (Bar?)arr[i % 3];
                if (local.HasValue)
                {
                    count += (int)local.Value;
                }
            }
            s_x = count;
        }

        static void AllTests(int iterationCount, Action<string> outputFunc)
        {
/*            Test(iterationCount, IsInstTestJustAPrimitive, nameof(IsInstTestJustAPrimitive), outputFunc);
            Test(iterationCount, IsInstTestJustObject, nameof(IsInstTestJustObject), outputFunc);
            Test(iterationCount, IsInstTestJust5Primitive, nameof(IsInstTestJust5Primitive), outputFunc);
            Test(iterationCount, IsInstTestJust5Object, nameof(IsInstTestJust5Object), outputFunc);
            Test(iterationCount, IsInstTestJust10Primitive, nameof(IsInstTestJust10Primitive), outputFunc);
            Test(iterationCount, IsInstTestJust10Object, nameof(IsInstTestJust10Object), outputFunc);

            Test(iterationCount, IsInstTestJustAPrimitiveNullableWithValue, nameof(IsInstTestJustAPrimitiveNullableWithValue), outputFunc);
            Test(iterationCount, IsInstTestJustObjectNullableWithValue, nameof(IsInstTestJustObjectNullableWithValue), outputFunc);*/
/*            Test(iterationCount, IsInstTestJust5PrimitiveNullableWithValue, nameof(IsInstTestJust5PrimitiveNullableWithValue), outputFunc);
            Test(iterationCount, IsInstTestJust5ObjectNullableWithValue, nameof(IsInstTestJust5ObjectNullableWithValue), outputFunc);
            Test(iterationCount, IsInstTestJust10PrimitiveNullableWithValue, nameof(IsInstTestJust10PrimitiveNullableWithValue), outputFunc);
            Test(iterationCount, IsInstTestJust10ObjectNullableWithValue, nameof(IsInstTestJust10ObjectNullableWithValue), outputFunc);

            Test(iterationCount, IsInstTestJustAPrimitiveNullNullable, nameof(IsInstTestJustAPrimitiveNullNullable), outputFunc);
            Test(iterationCount, IsInstTestJustObjectNullNullable, nameof(IsInstTestJustObjectNullNullable), outputFunc);
            Test(iterationCount, IsInstTestJust5PrimitiveNullNullable, nameof(IsInstTestJust5PrimitiveNullNullable), outputFunc);
            Test(iterationCount, IsInstTestJust5ObjectNullNullable, nameof(IsInstTestJust5ObjectNullNullable), outputFunc);
            Test(iterationCount, IsInstTestJust10PrimitiveNullNullable, nameof(IsInstTestJust10PrimitiveNullNullable), outputFunc);
            Test(iterationCount, IsInstTestJust10ObjectNullNullable, nameof(IsInstTestJust10ObjectNullNullable), outputFunc);

            Test(iterationCount, IsInstTestEnumVariance, nameof(IsInstTestEnumVariance), outputFunc);
//            Test(iterationCount, IsInstTestEnumVarianceNullable, nameof(IsInstTestEnumVarianceNullable), outputFunc);
*/

            Test(iterationCount, TestJustAPrimitive, nameof(TestJustAPrimitive), outputFunc);
            Test(iterationCount, TestJustObject, nameof(TestJustObject), outputFunc);
            Test(iterationCount, TestJust5Primitive, nameof(TestJust5Primitive), outputFunc);
            Test(iterationCount, TestJust5Object, nameof(TestJust5Object), outputFunc);
            Test(iterationCount, TestJust10Primitive, nameof(TestJust10Primitive), outputFunc);
            Test(iterationCount, TestJust10Object, nameof(TestJust10Object), outputFunc);

            Test(iterationCount, TestJustAPrimitiveNullableWithValue, nameof(TestJustAPrimitiveNullableWithValue), outputFunc);
            Test(iterationCount, TestJustObjectNullableWithValue, nameof(TestJustObjectNullableWithValue), outputFunc);
            Test(iterationCount, TestJust5PrimitiveNullableWithValue, nameof(TestJust5PrimitiveNullableWithValue), outputFunc);
            Test(iterationCount, TestJust5ObjectNullableWithValue, nameof(TestJust5ObjectNullableWithValue), outputFunc);
            Test(iterationCount, TestJust10PrimitiveNullableWithValue, nameof(TestJust10PrimitiveNullableWithValue), outputFunc);
            Test(iterationCount, TestJust10ObjectNullableWithValue, nameof(TestJust10ObjectNullableWithValue), outputFunc);

            Test(iterationCount, TestJustAPrimitiveNullNullable, nameof(TestJustAPrimitiveNullNullable), outputFunc);
            Test(iterationCount, TestJustObjectNullNullable, nameof(TestJustObjectNullNullable), outputFunc);
            Test(iterationCount, TestJust5PrimitiveNullNullable, nameof(TestJust5PrimitiveNullNullable), outputFunc);
            Test(iterationCount, TestJust5ObjectNullNullable, nameof(TestJust5ObjectNullNullable), outputFunc);
            Test(iterationCount, TestJust10PrimitiveNullNullable, nameof(TestJust10PrimitiveNullNullable), outputFunc);
            Test(iterationCount, TestJust10ObjectNullNullable, nameof(TestJust10ObjectNullNullable), outputFunc);
        }

        static void PrintNothing(string str) { }
        static void PrintLine(string str) { Console.Write(str); Console.Write("\n"); }

        static void Main(string[] args)
        {
            for (int i = 0; i < 100; i++)
            {
                AllTests(1, PrintNothing);
                Console.Write("");
            }

            Thread.Sleep(200);

            AllTests(500, PrintLine);
        }
    }
}

dotnet-policy-service · 2024-10-23T00:15:31Z

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

huoyaoyuan · 2024-10-23T03:11:36Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/BoxingHelpers.cs

+                }
+
+                // Set the hasValue field on the Nullable type. It MUST always be placed at the start of the object.
+                *(bool*)destPtr = true;


This doesn't look correct. The cast is casting the value of the reference into pointer. Unsafe.As would be correct.

Yeah, that seems wrong. I kept waffling between refs and pointers here. Thanks.

jkotas · 2024-10-23T13:34:30Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/BoxingHelpers.cs

+{
+    [StackTraceHidden]
+    [DebuggerStepThrough]
+    internal static unsafe partial class BoxingHelpers


Most of these types are going to be loaded on startup path. It feels a bit too fine grained to have separate type for the few unboxing helpers.

Would it be better for the unboxing helpers to live in CastHelpers? They are coupled anyway.

Sure, seems reasonable to me. I had thought I might be moving the allocation based helpers over too so this class wouldn't be so small, but honestly, just copying the assembly routines from NativeAOT seems like a better path for most of those.

…ue boxed nullable

src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs

jkotas · 2024-10-28T20:53:18Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs

+                // Set the hasValue field on the Nullable type. It MUST always be placed at the start of the object.
+                Unsafe.As<byte, bool>(ref destPtr) = true;
+                ref byte destValuePtr = ref typeMT->GetNullableValueFieldReferenceAndSize(ref destPtr, out uint size);
+                Unsafe.CopyBlockUnaligned(ref destValuePtr, ref RuntimeHelpers.GetRawData(obj), size);


Who does InitValueClass above need to be concerned with ref vs. non-ref differences, but this place does not need to be?

Because I haven't yet gotten around to fixing this problem... and am in the process of verifying if the alignment handling is necessary or not.

MichalPetryka · 2024-10-28T23:19:06Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs

+            MethodTable* pMT2 = RuntimeHelpers.GetMethodTable(obj);
+            if ((pMT1->IsPrimitive && pMT2->IsPrimitive &&
+                pMT1->GetPrimitiveCorElementType() == pMT2->GetPrimitiveCorElementType()) ||
+                AreTypesEquivalent(pMT1, pMT2))
+            {
+                return ref RuntimeHelpers.GetRawData(obj);
+            }
+
+            CastHelpers.ThrowInvalidCastException(obj, pMT1);
+            return ref Unsafe.AsRef<byte>(null);


Invert the if here to remove the unreachable return?

To do this, I expect that ThrowInvalidCastException would have to throw in C# to make the JIT realize that it is cold code and produce the desired code layout.

Ah, only now I've noticed that it does a QCall instead of direct throw. What's the point of the runtime call here?

Yes. And there are resource file changes that I would need to do to get the right string. I'll do it if you want @jkotas, but honestly this isn't so bad as it is. I couldn't find any impact on performance from the generated assembly code.

huoyaoyuan · 2024-10-29T02:41:49Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs

+            uint numInstanceFieldBytes = pMT->GetNumInstanceFieldBytes();
+            if ((((uint)Unsafe.AsPointer(ref destBytes) | numInstanceFieldBytes) & ((uint)sizeof(void*) - 1)) != 0)


Is is more or less efficient to check pMT->ContainsGCPointers?

I'm not sure. I'm not quite ready to benchmark this, and need to validate that we actually need to do aligned work here at all. I see 3 basic implementations.

This bifurcated approach, with some sort of if check on safety of using the general purpose clearing routine. That sort of thing is safe for cases where the destination may be on the heap or not. Using ContainsGCPointers wouldn't have been safe for the C++ implementation of all of this, but since this implementation of InitValueClass is actually only used for Nullable<T> instances, we CAN use that flag. I need to some performance investigation to determine the actual fastest approach here.

An approach where we p/invoke to 'C' memset as an FCall. That is only safe if the destination is guaranteed to be on the current stack. This needs to be an FCall since otherwise the specification of memset permits non-pointer atomic stores, and an inopportune GC suspension could see an invalid pointer.

A custom set routine which does a warmup series of sets to individual bytes until the region is aligned, then sets pointer sized chunks using atomic stores. (The CPU notion of atomic, not the C++ notion), and then has a warmdown phase. This is safe for all cases as well, and is what the old C++ implementation used to do.

A similar set of choices exists for the actual copying routine below.

In any case, my analysis of the usage of Unbox_Nullable is that usage for the JIT scenario always works with a stack based destination, but there is usage from Array.CoreCLR.cs which can unbox to the heap. I'm building a small suite of perf tests to see what the performance of various options in this space looks like.

…Services/CastHelpers.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>

…gHelpers

…nbox type testing

…ntime into UnboxingHelpers

src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs

+            MethodTable* pMT2 = RuntimeHelpers.GetMethodTable(obj);
+            if ((pMT1->IsPrimitive && pMT2->IsPrimitive &&
+                pMT1->GetPrimitiveCorElementType() == pMT2->GetPrimitiveCorElementType()) ||
+                AreTypesEquivalent(pMT1, pMT2))
+            {
+                return ref RuntimeHelpers.GetRawData(obj);
+            }
+
+            CastHelpers.ThrowInvalidCastException(obj, pMT1);
+            return ref Unsafe.AsRef<byte>(null);


- Fix assert that was always firing - Tweak code per code review to make the code generator aware that ThrowInvalidCastException is going to throw and should always be in a cold path - Improve the implementation of MethodTable.IsPrimitive. I noticed that this could be a single and+compare instead of the pair it was before. This improves the performance of Unbox_Helper slightly by about 5%.

jkotas · 2024-11-01T01:28:32Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs

+
+        [DebuggerHidden]
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        internal static void Unbox_TypeTest_Helper(MethodTable *pMT1, MethodTable *pMT2)


Suggested change

internal static void Unbox_TypeTest_Helper(MethodTable *pMT1, MethodTable *pMT2)

private static void Unbox_TypeTest_Helper(MethodTable *pMT1, MethodTable *pMT2)

jkotas · 2024-11-01T01:33:05Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs

+
+        [DebuggerHidden]
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        internal static ref byte Unbox_Helper(MethodTable* pMT1, object obj)


Suggested change

internal static ref byte Unbox_Helper(MethodTable* pMT1, object obj)

private static ref byte Unbox_Helper(MethodTable* pMT1, object obj)

jkotas · 2024-11-01T01:44:00Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs

+                else
+                {
+                    // If the type ContainsGCPointers, we can compute the size without resorting to loading the BaseSizePadding field from the EEClass
+                    nuint numInstanceFieldBytes = typeMT->BaseSize - (nuint)(2 * sizeof(IntPtr));


numInstanceFieldBytes local var is unusued. Is this unfinished refactoring?

It may be nice to move this to a property or method on MethodTable. There are more places where this micro-optimization can be used. The property impl can assert that this is only used on when ContainsGCPointers is true and that it returns the same value as full GetNumInstanceFieldBytes.

jkotas · 2024-11-01T01:49:20Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs

+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static void Unbox_Nullable_NotIsNullableForType(ref byte destPtr, MethodTable* typeMT, object obj)
+        {
+            // For safety's sake, also allow true nullables to be unboxed normally.


(I know that this is pre-existing comment.) This is not for safety's sake. We would not give up any safety if we threw for boxed Nullable<T> that should not exist here. It is just to hide bugs. Have you tried deleting this to see whether anything fails?

There are some cases in func eval which claim to use this pathway. In addition, it IS a pathway that has been used in reflection in the past, so I don't want to remove this path.

jkotas · 2024-11-01T01:53:59Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs

+                }
+                else
+                {
+                    UnboxNullableValue(ref destPtr, typeMT, obj);


What's the reason why we are not just calling BulkMoveWithWriteBarrier/Memmove here to copy the value? I would expect it to be faster.

It turns out that there is a fair amount of type system structure that would need to be ported to managed code to call BulkMoveWithWriteBarrier. I made the decision to pull this out into its own FCALL here. Especially since I expect that most paths will need to do a BulkMoveWithWriteBarrier which isn't that different in perf. I can explore pulling enough type system structure for this if you'd like me to.

Of course, now that I look at this, I realized that I can actually encode the needed bits for this particular load into the space used by m_pInterfaceMap which would allow all of this to avoid a couple of memory loads, branches etc. All of that should allow me to optimize the rest of all of this logic.

Just to make sure that I understand - is the main missing piece Nullable::ValueAddr?

Its ValueAddr + the computation of the size of the instance field bytes of the Value field. That requires access to the EEClass + access to a fielddesc, etc. It boils down to not that many instructions, but it IS a fair number of concepts and type system structures. That is a task probably worth doing at some point, but I'd like to avoid doing it now. The new encoding of data, which is redundant with existing data, allows for extremely cheap access, and is simple to encode into the managed code without adding more concepts than MethodTable.

… in appropriate places

jkotas · 2024-11-14T05:20:10Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs

+                }
+                else
+                {
+                    // If the type ContainsGCPointers, we can compute the size without resorting to loading the BaseSizePadding field from the EEClass


Suggested change

// If the type ContainsGCPointers, we can compute the size without resorting to loading the BaseSizePadding field from the EEClass

jkotas · 2024-11-14T05:20:36Z

...coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs

+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public uint GetNumInstanceFieldBytesIfContainsGCPointers()
+        {
+            Debug.Assert(ContainsGCPointers);


Suggested change

Debug.Assert(ContainsGCPointers);

// If the type ContainsGCPointers, we can compute the size without resorting to loading the BaseSizePadding field from the EEClass

Debug.Assert(ContainsGCPointers);

jkotas · 2024-11-14T05:24:24Z

...coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs

@@ -706,12 +703,41 @@ internal unsafe struct MethodTable
        [FieldOffset(ElementTypeOffset)]
        public void* ElementType;

+        /// <summary>
+        /// The PerInstInfo is used to describe the generic arguments and dictionary of this type.
+        /// It points as a PerInstInfo, which is an array of pointers to generic dictionaries, which then point


It points as a PerInstInfo

This does not parse for me.

I've updated the wording. Hopefully it is a bit more clear although for sure, the actual design of this is a very confusing structure.

jkotas · 2024-11-14T05:30:20Z

src/coreclr/vm/methodtablebuilder.cpp

+        if (pMT->IsValueType())
+        {
+            DWORD baseSizePadding = pMT->GetClass()->GetBaseSizePadding();
+            _ASSERTE(baseSizePadding == (sizeof(TADDR) * 2)); // This is dependended on by the System.Runtime.CompilerServices.CastHelpers.IsNullableForType code


I do not see the dependency in IsNullableForType. Obsolete comment?

I've updated the comment to reference GetNumInstanceFieldBytesIfContainsGCPointers

Move the unboxing helpers to managed code. Behavior is basically identical except for the Unbox_Nullable paths, which required some investigation to find the fastest implementation. Notably, there interruptibility of managed code makes the copying/zeroing of values more difficult, but with the opportunity/requirement to specialize the codebase came a few micro-optimizations that are somewhat nice. Overall I don't expect anyone to notice the performance changes here, but since my earlier code was about 2X slower than the native implementation, I did feel the need to optimize until everything looked good. Performance results: | TestName | With PR | Without PR | % Speedup | | --- | --- | --- | --- | | TestJustAPrimitive| 1217.0047| 1221.203| 0.34% | | TestJustObject| 1212.6415| 1211.6437|-0.08% | | TestJust5Primitive| 1496.5304| 1522.4968|1.74% | | TestJust5Object| 1461.0507| 1488.3328|1.87% | | TestJust10Primitive| 1473.2814| 1493.5238|1.37% | | TestJust10Object| 3215.6339| 2854.6186|-11.23% | | TestJustAPrimitiveNullableWithValue| 2727.9085| 5182.2611|89.97% | | TestJustObjectNullableWithValue| 3148.9484| 5672.2985|80.13% | | TestJust5PrimitiveNullableWithValue| 5443.9232| 7795.6109|43.20% | | TestJust5ObjectNullableWithValue| 6492.9071| 8095.1508|24.68% | | TestJust10PrimitiveNullableWithValue| 6022.6274| 8723.572| 44.85% | | TestJust10ObjectNullableWithValue| 7728.3239| 9671.1382|25.14% | | TestJustAPrimitiveNullNullable| 1786.1337| 2230.0932|24.86% | | TestJustObjectNullNullable| 1675.0683| 2326.0395|38.86% | | TestJust5PrimitiveNullNullable| 2921.9497| 3298.4642|12.89% | | TestJust5ObjectNullNullable| 3389.4043| 3615.3131|6.67% | | TestJust10PrimitiveNullNullable| 3050.809| 4054.9683|32.91% | | TestJust10ObjectNullNullable| 4658.8316| 5335.0686|14.52% | Results are very positive, or within the margin of error in this test suite. These results were generated using a small benchmark which mostly targeted measuring the performance of the Unbox_Nullable helper, as it has the most complex and potentially slow code. Generally the impact on that helper is that the performance of the type system portion of the helper is faster, and the performance of code which actually copies the contents of a valuetype is a little better. This isn't quite a fair test of managed vs native performance though, as I took the opportunity to restructure some of the memory on `MethodTable` so that it could more easily be read in managed code, and that happened to make a fair bit of complex code become simpler and thus faster.

Move unboxing helpers to managed code

c70a57c

dotnet-issue-labeler bot added the area-VM-coreclr label Oct 23, 2024

dotnet-policy-service bot assigned davidwrighton Oct 23, 2024

Add boxinghelpers.cs

f1258a4

huoyaoyuan reviewed Oct 23, 2024

View reviewed changes

jkotas reviewed Oct 23, 2024

View reviewed changes

davidwrighton added 2 commits October 24, 2024 10:10

Fix issues noted in CI/Review

02636f8

Fix more issues found in CI

d8e01e3

This was referenced Oct 24, 2024

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

davidwrighton added 2 commits October 25, 2024 10:58

Fix issue where UnboxNoCheck writes too many bytes when unboxing a tr…

b2c6991

…ue boxed nullable

Move unboxing helpers to CastHelpers class

12d17e5

jkotas reviewed Oct 28, 2024

View reviewed changes

MichalPetryka reviewed Oct 28, 2024

View reviewed changes

huoyaoyuan reviewed Oct 29, 2024

View reviewed changes

davidwrighton and others added 8 commits October 30, 2024 14:27

Before conversion to ref byte everywhere

3f281b9

It should all work now, and be fast

8dfaedc

Cleanup dead code

be624da

Update src/coreclr/System.Private.CoreLib/src/System/Runtime/Compiler…

dec085f

…Services/CastHelpers.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>

Merge branch 'main' of https://github.com/dotnet/runtime into Unboxin…

e22038b

…gHelpers

Fix build break

e44fab4

And finish up the details around optimizing for the fastest case in U…

2964105

…nbox type testing

Merge branch 'UnboxingHelpers' of https://github.com/davidwrighton/ru…

6469fbb

…ntime into UnboxingHelpers

This was referenced Oct 31, 2024

LibraryImportGenerator.Unit.Tests crashing on linux-x64 mono interpreter #100800

Open

[mono][interpreter] Mono interpreter is crashing during System.Data.Odbc.Tests (linux-x64 Release Mono_Interpreter_LibrariesTests) #101370

Closed

jkotas reviewed Oct 31, 2024

View reviewed changes

davidwrighton marked this pull request as ready for review October 31, 2024 21:18

jkotas reviewed Nov 1, 2024

View reviewed changes

davidwrighton added 3 commits November 13, 2024 13:07

Performance seeking behavior has finished. The perf is amazing

ee6d42d

Create GetNumInstanceFieldBytesIfContainsGCPointers helper and use it…

7761ad8

… in appropriate places

Last minute changes

55e0df9

jkotas approved these changes Nov 14, 2024

View reviewed changes

Update wording

6a8d1ae

davidwrighton merged commit eb456e6 into dotnet:main Nov 21, 2024
90 checks passed

am11 mentioned this pull request Nov 30, 2024

Remove Helper Method Frames (HMF) from Reflection #110211

Merged

This was referenced Dec 17, 2024

Removal of Helper Method Frames (HMF) #110767

Closed

Convert all FCalls with HELPER_METHOD_FRAME to QCalls #95695

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move unboxing helpers to managed code #109135

Move unboxing helpers to managed code #109135

davidwrighton commented Oct 23, 2024 •

edited

Loading

dotnet-policy-service bot commented Oct 23, 2024

huoyaoyuan Oct 23, 2024

davidwrighton Oct 24, 2024

jkotas Oct 23, 2024

davidwrighton Oct 28, 2024

jkotas Oct 28, 2024

davidwrighton Oct 28, 2024

MichalPetryka Oct 28, 2024

jkotas Oct 30, 2024

MichalPetryka Oct 30, 2024

davidwrighton Oct 30, 2024

This comment was marked as resolved.

huoyaoyuan Oct 29, 2024

davidwrighton Oct 29, 2024

davidwrighton Oct 29, 2024

This comment was marked as resolved.

jkotas Nov 1, 2024

jkotas Nov 1, 2024

jkotas Nov 1, 2024

jkotas Nov 1, 2024

davidwrighton Nov 13, 2024

jkotas Nov 1, 2024

davidwrighton Nov 12, 2024

davidwrighton Nov 12, 2024

jkotas Nov 12, 2024

davidwrighton Nov 13, 2024

jkotas Nov 14, 2024

jkotas Nov 14, 2024

jkotas Nov 14, 2024

davidwrighton Nov 19, 2024

jkotas Nov 14, 2024

davidwrighton Nov 19, 2024

		uint numInstanceFieldBytes = pMT->GetNumInstanceFieldBytes();
		if ((((uint)Unsafe.AsPointer(ref destBytes) \| numInstanceFieldBytes) & ((uint)sizeof(void*) - 1)) != 0)

	internal static void Unbox_TypeTest_Helper(MethodTable pMT1, MethodTable pMT2)
	private static void Unbox_TypeTest_Helper(MethodTable pMT1, MethodTable pMT2)

	internal static ref byte Unbox_Helper(MethodTable* pMT1, object obj)
	private static ref byte Unbox_Helper(MethodTable* pMT1, object obj)

Move unboxing helpers to managed code #109135

Move unboxing helpers to managed code #109135

Conversation

davidwrighton commented Oct 23, 2024 • edited Loading

dotnet-policy-service bot commented Oct 23, 2024

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

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

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

davidwrighton commented Oct 23, 2024 •

edited

Loading