[runtime] Move xamarin_create_managed_ref internal call to managed co…

…de. (#11271) Move the xamarin_create_managed_ref internal call to managed code, to ease things with CoreCLR. In order to preserve performance, this wasn't a straight forward port. * monotouch_create_managed_ref used to detect if there already was a GCHandle for a native object. To avoid a managed->native transition, this logic has now been moved into the code that sets the GCHandle (the xamarinSetGCHandle:flags: / xamarin_set_gchandle_trampoline code), and these methods return a value saying whether the GCHandle was set or not. * xamarin_create_gchandle will check the retain count to determine whether to create a weak or a strong GCHandle for the managed object. In this particular case we should never need to create a strong GCHandle, which means that we don't need to check the retain count (saving a managed->native transition). Using the new perftest (#11298), I get very similar numbers for both old code and new code: https://gist.github.com/rolfbjarne/e0fc2ae0f21da15062b4f051138679af (multiple runs). Sometimes the old code is faster, sometimes the new code is faster (although the old code tends to be the one who wins). In any case there aren't any significant performance hits due to this change, so it should be good to go.
xamarin · Apr 23, 2021 · 5d42c93 · 5d42c93 · vs-mobiletools-engineering-service2 · Apr 24, 2021
1 parent 9afd2aa
commit 5d42c93
Show file tree

Hide file tree

Showing 8 changed files with 77 additions and 51 deletions.
diff --git a/runtime/monovm-bridge.m b/runtime/monovm-bridge.m
@@ -125,8 +125,6 @@
 	nsvalue_class = get_class_from_name (platform_image, foundation, "NSValue", true);
 	nsstring_class = get_class_from_name (platform_image, foundation, "NSString", true);
 
-	xamarin_add_internal_call ("Foundation.NSObject::xamarin_create_managed_ref", (const void *) xamarin_create_managed_ref);
-
 	runtime_initialize = mono_class_get_method_from_name (runtime_class, "Initialize", 1);
 
 	if (runtime_initialize == NULL)

diff --git a/runtime/runtime.m b/runtime/runtime.m
@@ -479,7 +479,7 @@ void xamarin_framework_peer_unlock ()
 // compiler warning (no 'xamarinGetGChandle' selector found).
 @protocol XamarinExtendedObject
 -(GCHandle) xamarinGetGCHandle;
--(void) xamarinSetGCHandle: (GCHandle) gc_handle flags: (enum XamarinGCHandleFlags) flags;
+-(bool) xamarinSetGCHandle: (GCHandle) gc_handle flags: (enum XamarinGCHandleFlags) flags;
 -(enum XamarinGCHandleFlags) xamarinGetFlags;
 -(void) xamarinSetFlags: (enum XamarinGCHandleFlags) flags;
 @end
@@ -496,16 +496,34 @@ -(void) xamarinSetFlags: (enum XamarinGCHandleFlags) flags;
 	return rv;
 }
 
-static inline void
+static inline bool
 set_gchandle (id self, GCHandle gc_handle, enum XamarinGCHandleFlags flags)
 {
+	bool rv;
+
 	// COOP: we call a selector, and that must only be done in SAFE mode.
 	MONO_ASSERT_GC_UNSAFE;
 
 	MONO_ENTER_GC_SAFE;
 	id<XamarinExtendedObject> xself = self;
-	[xself xamarinSetGCHandle: gc_handle flags: flags];
+	rv = [xself xamarinSetGCHandle: gc_handle flags: flags];
 	MONO_EXIT_GC_SAFE;
+
+	return rv;
+}
+
+static inline bool
+set_gchandle_safe (id self, GCHandle gc_handle, enum XamarinGCHandleFlags flags)
+{
+	bool rv;
+
+	// COOP: we call a selector, and that must only be done in SAFE mode.
+	MONO_ASSERT_GC_SAFE_OR_DETACHED;
+
+	id<XamarinExtendedObject> xself = self;
+	rv = [xself xamarinSetGCHandle: gc_handle flags: flags];
+
+	return rv;
 }
 
 static inline GCHandle
@@ -1882,11 +1900,18 @@ -(void) xamarinSetFlags: (enum XamarinGCHandleFlags) flags;
 	set_gchandle (self, INVALID_GCHANDLE, XamarinGCHandleFlags_None);
 }
 
-void
+bool
 xamarin_set_gchandle_with_flags (id self, GCHandle gchandle, enum XamarinGCHandleFlags flags)
 {
 	// COOP: no managed memory access: any mode
-	set_gchandle (self, gchandle, flags);
+	return set_gchandle (self, gchandle, flags);
+}
+
+bool
+xamarin_set_gchandle_with_flags_safe (id self, GCHandle gchandle, enum XamarinGCHandleFlags flags)
+{
+	// COOP: no managed memory access: any mode
+	return set_gchandle_safe (self, gchandle, flags);
 }
 
 #if defined(DEBUG_REF_COUNTING)
@@ -1993,39 +2018,6 @@ -(void) xamarinSetFlags: (enum XamarinGCHandleFlags) flags;
 	[self release];
 }
 
-void
-xamarin_create_managed_ref (id self, gpointer managed_object, bool retain, bool user_type)
-{
-	// COOP: This is an icall, so at entry we're in unsafe mode.
-	// COOP: we stay in unsafe mode (since we write to the managed memory) unless calling a selector (which must be done in safe mode)
-	MONO_ASSERT_GC_UNSAFE;
-
-	GCHandle gchandle;
-
-#if defined(DEBUG_REF_COUNTING)
-	PRINT ("monotouch_create_managed_ref (%s Handle=%p) retainCount=%d; HasManagedRef=%i GCHandle=%i IsUserType=%i managed_object=%p\n", 
-		class_getName ([self class]), self, get_safe_retainCount (self), user_type ? xamarin_has_managed_ref (self) : 666, user_type ? get_gchandle_without_flags (self) : (void*) 666, user_type, managed_object);
-#endif
-
-	if (user_type) {
-		gchandle = get_gchandle_without_flags (self);
-		if (!gchandle) {
-			xamarin_create_gchandle (self, managed_object, XamarinGCHandleFlags_HasManagedRef, !retain);
-		} else {
-#if defined(DEBUG_REF_COUNTING)
-			xamarin_assertion_message ("GCHandle already exists for %p: %d\n", self, gchandle);
-#endif
-		}
-	}
-
-	if (retain) {
-		MONO_ENTER_GC_SAFE;
-		[self retain];
-		MONO_EXIT_GC_SAFE;
-	}
-	mt_dummy_use (managed_object);
-}
-
 /*
  * Block support
  */

diff --git a/runtime/trampolines.m b/runtime/trampolines.m
@@ -735,7 +735,7 @@
 }
 
 static const char *associated_key = "x"; // the string value doesn't matter, only the pointer value.
-void
+bool
 xamarin_set_gchandle_trampoline (id self, SEL sel, GCHandle gc_handle, enum XamarinGCHandleFlags flags)
 {
 	// COOP: Called by ObjC (when the setGCHandle:flags: selector is called on an object).
@@ -745,6 +745,13 @@
 	/* This is for types registered using the dynamic registrar */
 	XamarinAssociatedObject *obj;
 	obj = objc_getAssociatedObject (self, associated_key);
+
+	// Check if we're setting the initial value, in which case we don't want to overwrite
+	if (obj != NULL && obj->gc_handle != INVALID_GCHANDLE && ((flags & XamarinGCHandleFlags_InitialSet) == XamarinGCHandleFlags_InitialSet))
+		return false;
+
+	flags = (enum XamarinGCHandleFlags) (flags & ~XamarinGCHandleFlags_InitialSet); // Remove the InitialSet flag, we don't want to store it.
+
 	if (obj == NULL && gc_handle != INVALID_GCHANDLE) {
 		obj = [[XamarinAssociatedObject alloc] init];
 		obj->gc_handle = gc_handle;
@@ -774,6 +781,8 @@
 		CFDictionarySetValue (gchandle_hash, self, entry);
 	}
 	pthread_mutex_unlock (&gchandle_hash_lock);
+
+	return true;
 }
 
 GCHandle

diff --git a/runtime/xamarin/runtime.h b/runtime/xamarin/runtime.h
@@ -219,9 +219,9 @@ GCHandle		xamarin_get_gchandle (id self);
 void			xamarin_free_gchandle (id self, GCHandle gchandle);
 void			xamarin_clear_gchandle (id self);
 GCHandle		xamarin_get_gchandle_with_flags (id self, enum XamarinGCHandleFlags *flags);
-void			xamarin_set_gchandle_with_flags (id self, GCHandle gchandle, enum XamarinGCHandleFlags flags);
+bool			xamarin_set_gchandle_with_flags (id self, GCHandle gchandle, enum XamarinGCHandleFlags flags);
+bool			xamarin_set_gchandle_with_flags_safe (id self, GCHandle gchandle, enum XamarinGCHandleFlags flags);
 void			xamarin_create_gchandle (id self, void *managed_object, enum XamarinGCHandleFlags flags, bool force_weak);
-void			xamarin_create_managed_ref (id self, void * managed_object, bool retain, bool user_type);
 void            xamarin_release_managed_ref (id self, bool user_type);
 void			xamarin_notify_dealloc (id self, GCHandle gchandle);
 

diff --git a/runtime/xamarin/trampolines.h b/runtime/xamarin/trampolines.h
@@ -13,10 +13,12 @@
 extern "C" {
 #endif
 
+// Must be kept in sync with the same enum in NSObject2.cs
 enum XamarinGCHandleFlags : uint32_t {
 	XamarinGCHandleFlags_None = 0,
 	XamarinGCHandleFlags_WeakGCHandle = 1,
 	XamarinGCHandleFlags_HasManagedRef = 2,
+	XamarinGCHandleFlags_InitialSet = 4,
 };
 
 void *		xamarin_trampoline (id self, SEL sel, ...);
@@ -39,7 +41,7 @@ long long	xamarin_static_longret_trampoline (id self, SEL sel, ...);
 id			xamarin_copyWithZone_trampoline1 (id self, SEL sel, NSZone *zone);
 id			xamarin_copyWithZone_trampoline2 (id self, SEL sel, NSZone *zone);
 GCHandle	xamarin_get_gchandle_trampoline (id self, SEL sel);
-void		xamarin_set_gchandle_trampoline (id self, SEL sel, GCHandle gc_handle, enum XamarinGCHandleFlags flags);
+bool		xamarin_set_gchandle_trampoline (id self, SEL sel, GCHandle gc_handle, enum XamarinGCHandleFlags flags);
 enum XamarinGCHandleFlags xamarin_get_flags_trampoline (id self, SEL sel);
 void		xamarin_set_flags_trampoline (id self, SEL sel, enum XamarinGCHandleFlags flags);
 

diff --git a/src/Foundation/NSObject2.cs b/src/Foundation/NSObject2.cs
@@ -83,6 +83,14 @@ internal enum Flags : byte {
 			IsCustomType = 128,
 		}
 
+		// Must be kept in sync with the same enum in trampolines.h
+		enum XamarinGCHandleFlags : uint {
+			None = 0,
+			WeakGCHandle = 1,
+			HasManagedRef = 2,
+			InitialSet = 4,
+		}
+
 		[StructLayout (LayoutKind.Sequential)]
 		struct objc_super {
 			public IntPtr Handle;
@@ -205,9 +213,6 @@ internal static IntPtr Initialize ()
 		[DllImport ("__Internal")]
 		static extern void xamarin_release_managed_ref (IntPtr handle, bool user_type);
 
-		[MethodImplAttribute (MethodImplOptions.InternalCall)]
-		static extern void xamarin_create_managed_ref (IntPtr handle, NSObject obj, bool retain, bool user_type);
-
 #if !XAMCORE_3_0
 		public static bool IsNewRefcountEnabled ()
 		{
@@ -267,10 +272,26 @@ private void InitializeObject (bool alloced) {
 			CreateManagedRef (!alloced || native_ref);
 		}
 
+		[DllImport ("__Internal")]
+		static extern bool xamarin_set_gchandle_with_flags_safe (IntPtr handle, IntPtr gchandle, XamarinGCHandleFlags flags);
+
 		void CreateManagedRef (bool retain)
 		{
 			HasManagedRef = true;
-			xamarin_create_managed_ref (handle, this, retain, Runtime.IsUserType (handle));
+			bool isUserType = Runtime.IsUserType (handle);
+			if (isUserType) {
+				var flags = XamarinGCHandleFlags.HasManagedRef | XamarinGCHandleFlags.InitialSet | XamarinGCHandleFlags.WeakGCHandle;
+				var gchandle = GCHandle.Alloc (this, GCHandleType.WeakTrackResurrection);
+				var h = GCHandle.ToIntPtr (gchandle);
+				if (!xamarin_set_gchandle_with_flags_safe (handle, h, flags)) {
+					// A GCHandle already existed: this shouldn't happen, but let's handle it anyway.
+					Runtime.NSLog ("Tried to create a managed reference from an object that already has a managed reference (type: {0})", GetType ());
+					gchandle.Free ();
+				}
+			}
+
+			if (retain)
+				DangerousRetain ();
 		}
 
 		void ReleaseManagedRef ()

diff --git a/tools/common/StaticRegistrar.cs b/tools/common/StaticRegistrar.cs
@@ -2509,7 +2509,7 @@ string GetObjCSignature (ObjCMethod method, List<Exception> exceptions)
 			else if (method.CurrentTrampoline == Trampoline.SetFlags)
 				return "-(void) xamarinSetFlags: (enum XamarinGCHandleFlags) flags";
 			else if (method.CurrentTrampoline == Trampoline.SetGCHandle)
-				return "-(void) xamarinSetGCHandle: (GCHandle) gchandle flags: (enum XamarinGCHandleFlags) flags";
+				return "-(bool) xamarinSetGCHandle: (GCHandle) gchandle flags: (enum XamarinGCHandleFlags) flags";
 			else if (method.CurrentTrampoline == Trampoline.CopyWithZone1 || method.CurrentTrampoline == Trampoline.CopyWithZone2)
 				return "-(id) copyWithZone: (NSZone *)zone";
 
@@ -3139,11 +3139,16 @@ void Specialize (AutoIndentStringBuilder sb, ObjCMethod method, List<Exception>
 				sb.WriteLine ();
 				return;
 			case Trampoline.SetGCHandle:
-				sb.WriteLine ("-(void) xamarinSetGCHandle: (GCHandle) gc_handle flags: (enum XamarinGCHandleFlags) flags");
+				sb.WriteLine ("-(bool) xamarinSetGCHandle: (GCHandle) gc_handle flags: (enum XamarinGCHandleFlags) flags");
 				sb.WriteLine ("{");
+				sb.WriteLine ("if (((flags & XamarinGCHandleFlags_InitialSet) == XamarinGCHandleFlags_InitialSet) && __monoObjectGCHandle.gc_handle != INVALID_GCHANDLE) {");
+				sb.WriteLine ("return false;");
+				sb.WriteLine ("}");
+				sb.WriteLine ("flags = (enum XamarinGCHandleFlags) (flags & ~XamarinGCHandleFlags_InitialSet);"); // Remove the InitialSet flag, we don't want to store it.
 				sb.WriteLine ("__monoObjectGCHandle.gc_handle = gc_handle;");
 				sb.WriteLine ("__monoObjectGCHandle.flags = flags;");
 				sb.WriteLine ("__monoObjectGCHandle.native_object = self;");
+				sb.WriteLine ("return true;");
 				sb.WriteLine ("}");
 				sb.WriteLine ();
 				return;

diff --git a/tools/mtouch/Makefile b/tools/mtouch/Makefile
@@ -33,7 +33,6 @@ UNREFERENCED_SYMBOLS = 							\
 	_xamarin_IntPtr_objc_msgSend_IntPtr			\
 	_xamarin_IntPtr_objc_msgSendSuper_IntPtr	\
 	_xamarin_release_managed_ref				\
-	_xamarin_create_managed_ref					\
 	_xamarin_CGPoint__VNNormalizedFaceBoundingBoxPointForLandmarkPoint_Vector2_CGRect_nuint_nuint_string \
 	_xamarin_CGPoint__VNImagePointForFaceLandmarkPoint_Vector2_CGRect_nuint_nuint_string \
 	_CloseZStream								\