Improve Interlocked.Exchange<T> (#16058)

Replace TypedReference with Unsafe.As, it generates far less code and, with the help of AggresiveInlining, it allows Exchange<T> to inline.

4 files changed, 15 insertions, 91 deletions

diff --git a/src/mscorlib/src/System/Threading/Interlocked.cs b/src/mscorlib/src/System/Threading/Interlocked.cs
index 0a7968e5d7..898b0b1823 100644
--- a/src/mscorlib/src/System/Threading/Interlocked.cs
+++ b/src/mscorlib/src/System/Threading/Interlocked.cs
@@ -2,15 +2,9 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
-//
-
-using System;
 using System.Runtime.CompilerServices;
-using System.Runtime.ConstrainedExecution;
-using System.Runtime.Versioning;
-using System.Runtime;
+using Internal.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
-using System.Security;
 
 namespace System.Threading
 {
@@ -81,19 +75,14 @@ namespace System.Threading
         [MethodImplAttribute(MethodImplOptions.InternalCall)]
         public static extern IntPtr Exchange(ref IntPtr location1, IntPtr value);
 
+        // This whole method reduces to a single call to Exchange(ref object, object) but
+        // the JIT thinks that it will generate more native code than it actually does.
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public static T Exchange<T>(ref T location1, T value) where T : class
         {
-            _Exchange(__makeref(location1), __makeref(value));
-            //Since value is a local we use trash its data on return
-            //  The Exchange replaces the data with new data
-            //  so after the return "value" contains the original location1
-            //See ExchangeGeneric for more details           
-            return value;
+            return Unsafe.As<T>(Exchange(ref Unsafe.As<T, object>(ref location1), value));
         }
 
-        [MethodImplAttribute(MethodImplOptions.InternalCall)]
-        private static extern void _Exchange(TypedReference location1, TypedReference value);
-
         /******************************
          * CompareExchange
          *    Implemented: int
@@ -122,40 +111,20 @@ namespace System.Threading
         [MethodImplAttribute(MethodImplOptions.InternalCall)]
         public static extern IntPtr CompareExchange(ref IntPtr location1, IntPtr value, IntPtr comparand);
 
-        /*****************************************************************
-         * CompareExchange<T>
-         * 
-         * Notice how CompareExchange<T>() uses the __makeref keyword
-         * to create two TypedReferences before calling _CompareExchange().
-         * This is horribly slow. Ideally we would like CompareExchange<T>()
-         * to simply call CompareExchange(ref Object, Object, Object); 
-         * however, this would require casting a "ref T" into a "ref Object", 
-         * which is not legal in C#.
-         * 
-         * Thus we opted to implement this in the JIT so that when it reads
-         * the method body for CompareExchange<T>() it gets back the
-         * following IL:
-         *
-         *     ldarg.0 
-         *     ldarg.1
-         *     ldarg.2
-         *     call System.Threading.Interlocked::CompareExchange(ref Object, Object, Object)
-         *     ret
-         *
-         * See getILIntrinsicImplementationForInterlocked() in VM\JitInterface.cpp
-         * for details.
-         *****************************************************************/
-
+        // Note that getILIntrinsicImplementationForInterlocked() in vm\jitinterface.cpp replaces
+        // the body of this method with the the following IL:
+        //     ldarg.0 
+        //     ldarg.1
+        //     ldarg.2
+        //     call System.Threading.Interlocked::CompareExchange(ref Object, Object, Object)
+        //     ret
+        // The workaround is no longer strictly necessary now that we have Unsafe.As but it does
+        // have the advantage of being less sensitive to JIT's inliner decisions.
         public static T CompareExchange<T>(ref T location1, T value, T comparand) where T : class
         {
-            // _CompareExchange() passes back the value read from location1 via local named 'value'
-            _CompareExchange(__makeref(location1), __makeref(value), comparand);
-            return value;
+            return Unsafe.As<T>(CompareExchange(ref Unsafe.As<T, object>(ref location1), value, comparand));
         }
 
-        [MethodImplAttribute(MethodImplOptions.InternalCall)]
-        private static extern void _CompareExchange(TypedReference location1, TypedReference value, Object comparand);
-
         // BCL-internal overload that returns success via a ref bool param, useful for reliable spin locks.
         [MethodImplAttribute(MethodImplOptions.InternalCall)]
         internal static extern int CompareExchange(ref int location1, int value, int comparand, ref bool succeeded);
diff --git a/src/vm/comutilnative.cpp b/src/vm/comutilnative.cpp
index f9f159a61a..c4563992b7 100644
--- a/src/vm/comutilnative.cpp
+++ b/src/vm/comutilnative.cpp
@@ -1807,46 +1807,6 @@ FCIMPL2(LPVOID,COMInterlocked::ExchangeObject, LPVOID*location, LPVOID value)
 }
 FCIMPLEND
 
-FCIMPL2_VV(void,COMInterlocked::ExchangeGeneric, FC_TypedByRef location, FC_TypedByRef value)
-{
-    FCALL_CONTRACT;
-
-    LPVOID* loc = (LPVOID*)location.data;
-    if( NULL == loc) {
-        FCThrowVoid(kNullReferenceException);
-    }
-
-    LPVOID val = *(LPVOID*)value.data;
-    *(LPVOID*)value.data = FastInterlockExchangePointer(loc, val);
-#ifdef _DEBUG
-    Thread::ObjectRefAssign((OBJECTREF *)loc);
-#endif
-    ErectWriteBarrier((OBJECTREF*) loc, ObjectToOBJECTREF((Object*) val));
-}
-FCIMPLEND
-
-FCIMPL3_VVI(void,COMInterlocked::CompareExchangeGeneric, FC_TypedByRef location, FC_TypedByRef value, LPVOID comparand)
-{
-    FCALL_CONTRACT;
-
-    LPVOID* loc = (LPVOID*)location.data;
-    LPVOID val = *(LPVOID*)value.data;
-    if( NULL == loc) {
-        FCThrowVoid(kNullReferenceException);
-    }
-
-    LPVOID ret = FastInterlockCompareExchangePointer(loc, val, comparand);
-    *(LPVOID*)value.data = ret;
-    if(ret == comparand)
-    {
-#ifdef _DEBUG
-        Thread::ObjectRefAssign((OBJECTREF *)loc);
-#endif
-        ErectWriteBarrier((OBJECTREF*) loc, ObjectToOBJECTREF((Object*) val));
-    }
-}
-FCIMPLEND
-
 FCIMPL3(LPVOID,COMInterlocked::CompareExchangeObject, LPVOID *location, LPVOID value, LPVOID comparand)
 {
     FCALL_CONTRACT;
diff --git a/src/vm/comutilnative.h b/src/vm/comutilnative.h
index b1e47bef21..617785de94 100644
--- a/src/vm/comutilnative.h
+++ b/src/vm/comutilnative.h
@@ -193,8 +193,6 @@ public:
         static FCDECL3(LPVOID, CompareExchangeObject, LPVOID* location, LPVOID value, LPVOID comparand);
         static FCDECL2(INT32, ExchangeAdd32, INT32 *location, INT32 value);
         static FCDECL2_IV(INT64, ExchangeAdd64, INT64 *location, INT64 value);
-        static FCDECL2_VV(void, ExchangeGeneric, FC_TypedByRef location, FC_TypedByRef value);
-        static FCDECL3_VVI(void, CompareExchangeGeneric, FC_TypedByRef location, FC_TypedByRef value, LPVOID comparand);
 
         static FCDECL0(void, FCMemoryBarrier);
         static void QCALLTYPE MemoryBarrierProcessWide();
diff --git a/src/vm/ecalllist.h b/src/vm/ecalllist.h
index 7cd6889c47..525227b7c3 100644
--- a/src/vm/ecalllist.h
+++ b/src/vm/ecalllist.h
@@ -982,9 +982,6 @@ FCFuncStart(gInterlockedFuncs)
     FCIntrinsicSig("ExchangeAdd", &gsig_SM_RefInt_Int_RetInt, COMInterlocked::ExchangeAdd32, CORINFO_INTRINSIC_InterlockedXAdd32)
     FCIntrinsicSig("ExchangeAdd", &gsig_SM_RefLong_Long_RetLong, COMInterlocked::ExchangeAdd64, CORINFO_INTRINSIC_InterlockedXAdd64)
 
-    FCFuncElement("_Exchange", COMInterlocked::ExchangeGeneric)
-    FCFuncElement("_CompareExchange", COMInterlocked::CompareExchangeGeneric)
-
     FCIntrinsic("MemoryBarrier", COMInterlocked::FCMemoryBarrier, CORINFO_INTRINSIC_MemoryBarrier)
     QCFuncElement("_MemoryBarrierProcessWide", COMInterlocked::MemoryBarrierProcessWide)
 FCFuncEnd()