Improve the performance of ConditionalWeakTable.TryGetValue (#80059)

* [NativeAOT] Fix Objective-C reference tracking

Fixes #80032

* Move implementation-specific comment out of public doc comment

* Duplicate code for TryGetHashCode implementations.

* Replace comments with a passing test.

* Add moke test for restricted callouts.

* Remove TryGetHashCode from Mono

It does not guarantee that hash codes are non-zero.

* Add test coverage for untracked objective objects.

* Implement RuntimeHelpers.TryGetHashCode for Mono

* Remove unneeded MONO_COMPONENT_API

* Remove Mono intrinsic for InternalGetHashCode

This is dead code because this method no longer lives on
System.Object.

* Move interpreter transforms to correct class.

* Rename and move icall to match convention.

Co-authored-by: Jan Kotas <[email protected]>

Author: AustinWise

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

@@ -114,6 +114,17 @@ public static unsafe void PrepareMethod(RuntimeMethodHandle method, RuntimeTypeH
         [MethodImpl(MethodImplOptions.InternalCall)]
         public static extern int GetHashCode(object? o);
 
+        /// <summary>
+        /// If a hash code has been assigned to the object, it is returned. Otherwise zero is
+        /// returned.
+        /// </summary>
+        /// <remarks>
+        /// The advantage of this over <see cref="GetHashCode" /> is that it avoids assigning a hash
+        /// code to the object if it does not already have one.
+        /// </remarks>
+        [MethodImpl(MethodImplOptions.InternalCall)]
+        internal static extern int TryGetHashCode(object o);
+
         [MethodImpl(MethodImplOptions.InternalCall)]
         public static extern new bool Equals(object? o1, object? o2);
 

src/coreclr/classlibnative/bcltype/objectnative.cpp

@@ -128,6 +128,47 @@ FCIMPL1(INT32, ObjectNative::GetHashCode, Object* obj) {
 }
 FCIMPLEND
 
+FCIMPL1(INT32, ObjectNative::TryGetHashCode, Object* obj) {
+
+    CONTRACTL
+    {
+        FCALL_CHECK;
+    }
+    CONTRACTL_END;
+
+    VALIDATEOBJECT(obj);
+
+    if (obj == 0)
+        return 0;
+
+    OBJECTREF objRef(obj);
+
+    {
+        DWORD bits = objRef->GetHeader()->GetBits();
+
+        if (bits & BIT_SBLK_IS_HASH_OR_SYNCBLKINDEX)
+        {
+            if (bits & BIT_SBLK_IS_HASHCODE)
+            {
+                // Common case: the object already has a hash code
+                return  bits & MASK_HASHCODE;
+            }
+            else
+            {
+                // We have a sync block index. There may be a hash code stored within the sync block.
+                SyncBlock *psb = objRef->PassiveGetSyncBlock();
+                if (psb != NULL)
+                {
+                    return psb->GetHashCode();
+                }
+            }
+        }
+    }
+
+    return 0;
+}
+FCIMPLEND
+
 //
 // Compare by ref for normal classes, by value for value types.
 //

src/coreclr/classlibnative/bcltype/objectnative.h

@@ -30,6 +30,7 @@ class ObjectNative
     //  If the Class object doesn't exist then you must call the GetClass() method.
     static FCDECL1(Object*, GetObjectValue, Object* vThisRef);
     static FCDECL1(INT32, GetHashCode, Object* vThisRef);
+    static FCDECL1(INT32, TryGetHashCode, Object* vThisRef);
     static FCDECL2(FC_BOOL_RET, Equals, Object *pThisRef, Object *pCompareRef);
     static FCDECL1(Object*, AllocateUninitializedClone, Object* pObjUNSAFE);
     static FCDECL1(Object*, GetClass, Object* pThis);

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.NativeAot.cs

@@ -104,6 +104,19 @@ public static unsafe int GetHashCode(object o)
             return ObjectHeader.GetHashCode(o);
         }
 
+        /// <summary>
+        /// If a hash code has been assigned to the object, it is returned. Otherwise zero is
+        /// returned.
+        /// </summary>
+        /// <remarks>
+        /// The advantage of this over <see cref="GetHashCode" /> is that it avoids assigning a hash
+        /// code to the object if it does not already have one.
+        /// </remarks>
+        internal static int TryGetHashCode(object o)
+        {
+            return ObjectHeader.TryGetHashCode(o);
+        }
+
         [Obsolete("OffsetToStringData has been deprecated. Use string.GetPinnableReference() instead.")]
         public static int OffsetToStringData
         {

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Threading/ObjectHeader.cs

@@ -85,6 +85,38 @@ public static unsafe int GetHashCode(object o)
             }
         }
 
+        /// <summary>
+        /// If a hash code has been assigned to the object, it is returned. Otherwise zero is
+        /// returned.
+        /// </summary>
+        public static unsafe int TryGetHashCode(object o)
+        {
+            if (o == null)
+                return 0;
+
+            fixed (MethodTable** ppMethodTable = &o.GetMethodTableRef())
+            {
+                int* pHeader = GetHeaderPtr(ppMethodTable);
+                int bits = *pHeader;
+                int hashOrIndex = bits & MASK_HASHCODE_INDEX;
+                if ((bits & BIT_SBLK_IS_HASHCODE) != 0)
+                {
+                    // Found the hash code in the header
+                    Debug.Assert(hashOrIndex != 0);
+                    return hashOrIndex;
+                }
+
+                if ((bits & BIT_SBLK_IS_HASH_OR_SYNCBLKINDEX) != 0)
+                {
+                    // Look up the hash code in the SyncTable
+                    return SyncTable.GetHashCode(hashOrIndex);
+                }
+
+                // The hash code has not yet been set.
+                return 0;
+            }
+        }
+
         /// <summary>
         /// Assigns a hash code to the object in a thread-safe way.
         /// </summary>

src/coreclr/vm/ecalllist.h

@@ -552,6 +552,7 @@ FCFuncStart(gRuntimeHelpers)
     FCFuncElement("GetSpanDataFrom", ArrayNative::GetSpanDataFrom)
     FCFuncElement("PrepareDelegate", ReflectionInvocation::PrepareDelegate)
     FCFuncElement("GetHashCode", ObjectNative::GetHashCode)
+    FCFuncElement("TryGetHashCode", ObjectNative::TryGetHashCode)
     FCFuncElement("Equals", ObjectNative::Equals)
     FCFuncElement("AllocateUninitializedClone", ObjectNative::AllocateUninitializedClone)
     FCFuncElement("EnsureSufficientExecutionStack", ReflectionInvocation::EnsureSufficientExecutionStack)

src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConditionalWeakTable.cs

@@ -48,7 +48,7 @@ public ConditionalWeakTable()
         /// </param>
         /// <returns>Returns "true" if key was found, "false" otherwise.</returns>
         /// <remarks>
-        /// The key may get garbaged collected during the TryGetValue operation. If so, TryGetValue
+        /// The key may get garbage collected during the TryGetValue operation. If so, TryGetValue
         /// may at its discretion, return "false" and set "value" to the default (as if the key was not present.)
         /// </remarks>
         public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value)
@@ -538,7 +538,17 @@ internal int FindEntry(TKey key, out object? value)
             {
                 Debug.Assert(key != null); // Key already validated as non-null.
 
-                int hashCode = RuntimeHelpers.GetHashCode(key) & int.MaxValue;
+                int hashCode = RuntimeHelpers.TryGetHashCode(key);
+
+                if (hashCode == 0)
+                {
+                    // No hash code has been assigned to the key, so therefore it has not been added
+                    // to any ConditionalWeakTable.
+                    value = null;
+                    return -1;
+                }
+
+                hashCode &= int.MaxValue;
                 int bucket = hashCode & (_buckets.Length - 1);
                 for (int entriesIndex = Volatile.Read(ref _buckets[bucket]); entriesIndex != -1; entriesIndex = _entries[entriesIndex].Next)
                 {

src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs

@@ -44,6 +44,22 @@ public static int GetHashCode(object? o)
             return InternalGetHashCode(o);
         }
 
+        [MethodImplAttribute(MethodImplOptions.InternalCall)]
+        private static extern int InternalTryGetHashCode(object? o);
+
+        /// <summary>
+        /// If a hash code has been assigned to the object, it is returned. Otherwise zero is
+        /// returned.
+        /// </summary>
+        /// <remarks>
+        /// The advantage of this over <see cref="GetHashCode" /> is that it avoids assigning a hash
+        /// code to the object if it does not already have one.
+        /// </remarks>
+        public static int TryGetHashCode(object? o)
+        {
+            return InternalTryGetHashCode(o);
+        }
+
         public static new bool Equals(object? o1, object? o2)
         {
             if (o1 == o2)

src/mono/mono/metadata/icall-def.h

@@ -427,7 +427,8 @@ HANDLES(RUNH_1, "GetObjectValue", ves_icall_System_Runtime_CompilerServices_Runt
 HANDLES(RUNH_6, "GetSpanDataFrom", ves_icall_System_Runtime_CompilerServices_RuntimeHelpers_GetSpanDataFrom, gpointer, 3, (MonoClassField_ptr, MonoType_ptr, gpointer))
 HANDLES(RUNH_2, "GetUninitializedObjectInternal", ves_icall_System_Runtime_CompilerServices_RuntimeHelpers_GetUninitializedObjectInternal, MonoObject, 1, (MonoType_ptr))
 HANDLES(RUNH_3, "InitializeArray", ves_icall_System_Runtime_CompilerServices_RuntimeHelpers_InitializeArray, void, 2, (MonoArray, MonoClassField_ptr))
-HANDLES(RUNH_7, "InternalGetHashCode", mono_object_hash_icall, int, 1, (MonoObject))
+HANDLES(RUNH_7, "InternalGetHashCode", ves_icall_System_Runtime_CompilerServices_RuntimeHelpers_InternalGetHashCode, int, 1, (MonoObject))
+HANDLES(RUNH_8, "InternalTryGetHashCode", ves_icall_System_Runtime_CompilerServices_RuntimeHelpers_InternalTryGetHashCode, int, 1, (MonoObject))
 HANDLES(RUNH_3a, "PrepareMethod", ves_icall_System_Runtime_CompilerServices_RuntimeHelpers_PrepareMethod, void, 3, (MonoMethod_ptr, gpointer, int))
 HANDLES(RUNH_4, "RunClassConstructor", ves_icall_System_Runtime_CompilerServices_RuntimeHelpers_RunClassConstructor, void, 1, (MonoType_ptr))
 HANDLES(RUNH_5, "RunModuleConstructor", ves_icall_System_Runtime_CompilerServices_RuntimeHelpers_RunModuleConstructor, void, 1, (MonoImage_ptr))

src/mono/mono/metadata/icall.c

@@ -1072,6 +1072,18 @@ ves_icall_System_Runtime_CompilerServices_RuntimeHelpers_InitializeArray (MonoAr
 #endif
 }
 
+int
+ves_icall_System_Runtime_CompilerServices_RuntimeHelpers_InternalGetHashCode (MonoObjectHandle obj, MonoError* error)
+{
+	return mono_object_hash_internal (MONO_HANDLE_RAW (obj));
+}
+
+int
+ves_icall_System_Runtime_CompilerServices_RuntimeHelpers_InternalTryGetHashCode (MonoObjectHandle obj, MonoError* error)
+{
+	return mono_object_try_get_hash_internal (MONO_HANDLE_RAW (obj));
+}
+
 MonoObjectHandle
 ves_icall_System_Runtime_CompilerServices_RuntimeHelpers_GetObjectValue (MonoObjectHandle obj, MonoError *error)
 {

src/mono/mono/metadata/monitor.c

@@ -515,6 +515,12 @@ mono_monitor_inflate (MonoObject *obj)
 
 #define MONO_OBJECT_ALIGNMENT_SHIFT	3
 
+/*
+ * Wang's address-based hash function:
+ *   http://www.concentric.net/~Ttwang/tech/addrhash.htm
+ */
+#define HASH_OBJECT(obj) (GPOINTER_TO_UINT (obj) >> MONO_OBJECT_ALIGNMENT_SHIFT) * 2654435761u
+
 int
 mono_object_hash_internal (MonoObject* obj)
 {
@@ -542,11 +548,14 @@ mono_object_hash_internal (MonoObject* obj)
 	 * another thread computes the hash at the same time, because it'll end up
 	 * with the same value.
 	 */
-	hash = (GPOINTER_TO_UINT (obj) >> MONO_OBJECT_ALIGNMENT_SHIFT) * 2654435761u;
+	hash = HASH_OBJECT(obj);
 #if SIZEOF_VOID_P == 4
 	/* clear the top bits as they can be discarded */
 	hash &= ~(LOCK_WORD_STATUS_MASK << (32 - LOCK_WORD_STATUS_BITS));
 #endif
+	if (hash == 0) {
+		hash = 1;
+	}
 	if (lock_word_is_free (lw)) {
 		LockWord old_lw;
 		lw = lock_word_new_thin_hash (hash);
@@ -581,19 +590,48 @@ mono_object_hash_internal (MonoObject* obj)
 
 #else
 
-/*
- * Wang's address-based hash function:
- *   http://www.concentric.net/~Ttwang/tech/addrhash.htm
- */
-	return (GPOINTER_TO_UINT (obj) >> MONO_OBJECT_ALIGNMENT_SHIFT) * 2654435761u;
+	unsigned int hash = HASH_OBJECT(obj);
+	if (hash == 0) {
+		hash = 1;
+	}
+	return hash;
+
 #endif
 
 }
 
 int
-mono_object_hash_icall (MonoObjectHandle obj, MonoError* error)
+mono_object_try_get_hash_internal (MonoObject* obj)
 {
-	return mono_object_hash_internal (MONO_HANDLE_RAW (obj));
+#ifdef HAVE_MOVING_COLLECTOR
+
+	LockWord lw;
+	if (!obj)
+		return 0;
+	lw.sync = obj->synchronisation;
+
+	LOCK_DEBUG (g_message("%s: (%d) Get hash for object %p; LW = %p", __func__, mono_thread_info_get_small_id (), obj, obj->synchronisation));
+
+	if (lock_word_has_hash (lw)) {
+		if (lock_word_is_inflated (lw)) {
+			return lock_word_get_inflated_lock (lw)->hash_code;
+		} else {
+			return lock_word_get_hash (lw);
+		}
+	}
+
+	return 0;
+
+#else
+
+	unsigned int hash = HASH_OBJECT(obj);
+	if (hash == 0) {
+		hash = 1;
+	}
+	return hash;
+
+#endif
+
 }
 
 /*

src/mono/mono/metadata/object-internals.h

@@ -1997,6 +1997,9 @@ mono_string_hash_internal (MonoString *s);
 MONO_COMPONENT_API int
 mono_object_hash_internal (MonoObject* obj);
 
+int
+mono_object_try_get_hash_internal (MonoObject* obj);
+
 ICALL_EXPORT
 void
 mono_value_copy_internal (void* dest, const void* src, MonoClass *klass);

src/mono/mono/mini/interp/interp.c

@@ -7510,6 +7510,11 @@ MINT_IN_CASE(MINT_BRTRUE_I8_SP) ZEROP_SP(gint64, !=); MINT_IN_BREAK;
 			ip += 3;
 			MINT_IN_BREAK;
 		}
+		MINT_IN_CASE(MINT_INTRINS_TRY_GET_HASHCODE) {
+			LOCAL_VAR (ip [1], gint32) = mono_object_try_get_hash_internal (LOCAL_VAR (ip [2], MonoObject*));
+			ip += 3;
+			MINT_IN_BREAK;
+		}
 		MINT_IN_CASE(MINT_INTRINS_GET_TYPE) {
 			MonoObject *o = LOCAL_VAR (ip [2], MonoObject*);
 			NULL_CHECK (o);

src/mono/mono/mini/interp/mintops.def

@@ -794,6 +794,7 @@ OPDEF(MINT_TIER_PATCHPOINT, "tier_patchpoint", 2, 0, 0, MintOpShortInt)
 
 OPDEF(MINT_INTRINS_ENUM_HASFLAG, "intrins_enum_hasflag", 5, 1, 2, MintOpClassToken)
 OPDEF(MINT_INTRINS_GET_HASHCODE, "intrins_get_hashcode", 3, 1, 1, MintOpNoArgs)
+OPDEF(MINT_INTRINS_TRY_GET_HASHCODE, "intrins_try_get_hashcode", 3, 1, 1, MintOpNoArgs)
 OPDEF(MINT_INTRINS_GET_TYPE, "intrins_get_type", 3, 1, 1, MintOpNoArgs)
 OPDEF(MINT_INTRINS_SPAN_CTOR, "intrins_span_ctor", 4, 1, 2, MintOpNoArgs)
 OPDEF(MINT_INTRINS_UNSAFE_BYTE_OFFSET, "intrins_unsafe_byte_offset", 4, 1, 2, MintOpNoArgs)

src/mono/mono/mini/interp/transform.c

@@ -2368,6 +2368,10 @@ interp_handle_intrinsics (TransformData *td, MonoMethod *target_method, MonoClas
 			interp_ins_set_dreg (td->last_ins, td->sp [-1].local);
 			td->ip += 5;
 			return TRUE;
+		} else if (!strcmp (tm, "InternalGetHashCode")) {
+			*op = MINT_INTRINS_GET_HASHCODE;
+		} else if (!strcmp (tm, "InternalTryGetHashCode")) {
+			*op = MINT_INTRINS_TRY_GET_HASHCODE;
 		} else if (!strcmp (tm, "GetRawData")) {
 			interp_add_ins (td, MINT_LDFLDA_UNSAFE);
 			td->last_ins->data [0] = (gint16) MONO_ABI_SIZEOF (MonoObject);
@@ -2426,9 +2430,7 @@ interp_handle_intrinsics (TransformData *td, MonoMethod *target_method, MonoClas
 			td->sp [-1].klass == mono_defaults.runtimetype_class && td->sp [-2].klass == mono_defaults.runtimetype_class) {
 		*op = MINT_CNE_P;
 	} else if (in_corlib && target_method->klass == mono_defaults.object_class) {
-		if (!strcmp (tm, "InternalGetHashCode")) {
-			*op = MINT_INTRINS_GET_HASHCODE;
-		} else if (!strcmp (tm, "GetType")) {
+		if (!strcmp (tm, "GetType")) {
 			if (constrained_class && m_class_is_valuetype (constrained_class) && !mono_class_is_nullable (constrained_class)) {
 				// If constrained_class is valuetype we already know its type.
 				// Resolve GetType to a constant so we can fold type comparisons

src/mono/mono/mini/intrinsics.c

@@ -877,15 +877,6 @@ mini_emit_inst_for_method (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSign
 			mini_type_to_eval_stack_type (cfg, fsig->ret, ins);
 			ins->klass = mono_defaults.runtimetype_class;
 			*ins_type_initialized = TRUE;
-			return ins;
-		} else if (!cfg->backend->emulate_mul_div && strcmp (cmethod->name, "InternalGetHashCode") == 0 && fsig->param_count == 1 && !mono_gc_is_moving ()) {
-			int dreg = alloc_ireg (cfg);
-			int t1 = alloc_ireg (cfg);
-
-			MONO_EMIT_NEW_BIALU_IMM (cfg, OP_SHR_IMM, t1, args [0]->dreg, 3);
-			EMIT_NEW_BIALU_IMM (cfg, ins, OP_MUL_IMM, dreg, t1, 2654435761u);
-			ins->type = STACK_I4;
-
 			return ins;
 		} else if (strcmp (cmethod->name, ".ctor") == 0 && fsig->param_count == 0) {
  			MONO_INST_NEW (cfg, ins, OP_NOP);