Use a macro for simdhash ght key comparer and key hasher since it seems like sometimes they don't get inlined

Add some measurements for simdhash ght

Improve makefile

Introduce scan data that's fetched once per bucket

Targeted simdhash optimizations via new dn_simdhash_ght_get_value_or_default

Fix indentation to satisfy CI build warnings

Fix build warnings

Switch to spaced prime bucket counts instead of power of two bucket counts for better collision resistance

Fix cache alignment of ght compatible and ptr ptr simdhash variants on 64-bit

Higher simdhash load factor (closer to ghashtable's; improves performance for bad hashes)

Improve benchmark overhead calculation

Add sequential keys measurements

Fix build on CI

Revert ght bucket size

Revert "Revert ght bucket size"

This reverts commit 2a225ad.

Bug fixes
Add murmurhash32 variant for use in improved mono hash

Increase load factor to 30%

Author: kg

src/mono/mono/metadata/bundled-resources.c

@@ -204,9 +204,7 @@ bundled_resources_get (const char *id)
 	char key_buffer[1024];
 	key_from_id(id, key_buffer, sizeof(key_buffer));
 
-	MonoBundledResource *result = NULL;
-	dn_simdhash_ght_try_get_value (bundled_resources, key_buffer, (void **)&result);
-	return result;
+	return (MonoBundledResource *)dn_simdhash_ght_get_value_or_default (bundled_resources, key_buffer);
 }
 
 //---------------------------------------------------------------------------------------

src/mono/mono/metadata/class.c

@@ -1252,7 +1252,7 @@ mono_class_inflate_generic_method_full_checked (MonoMethod *method, MonoClass *k
 	mono_mem_manager_lock (mm);
 	if (!mm->gmethod_cache)
 		mm->gmethod_cache = dn_simdhash_ght_new_full (inflated_method_hash, inflated_method_equal, NULL, (GDestroyNotify)free_inflated_method, 0, NULL);
-	dn_simdhash_ght_try_get_value (mm->gmethod_cache, iresult, (void **)&cached);
+	cached = dn_simdhash_ght_get_value_or_default (mm->gmethod_cache, iresult);
 	mono_mem_manager_unlock (mm);
 
 	if (cached) {
@@ -1358,8 +1358,7 @@ mono_class_inflate_generic_method_full_checked (MonoMethod *method, MonoClass *k
 
 	// check cache
 	mono_mem_manager_lock (mm);
-	cached = NULL;
-	dn_simdhash_ght_try_get_value (mm->gmethod_cache, iresult, (void **)&cached);
+	cached = dn_simdhash_ght_get_value_or_default (mm->gmethod_cache, iresult);
 	if (!cached) {
 		dn_simdhash_ght_insert (mm->gmethod_cache, iresult, iresult);
 		iresult->owner = mm;

src/mono/mono/metadata/metadata.c

@@ -3359,7 +3359,7 @@ mono_metadata_get_inflated_signature (MonoMethodSignature *sig, MonoGenericConte
 		mm->gsignature_cache = dn_simdhash_ght_new_full (inflated_signature_hash, inflated_signature_equal, NULL, (GDestroyNotify)free_inflated_signature, 256, NULL);
 
 	// FIXME: The lookup is done on the newly allocated sig so it always fails
-	dn_simdhash_ght_try_get_value (mm->gsignature_cache, &helper, (gpointer *)&res);
+	res = dn_simdhash_ght_get_value_or_default (mm->gsignature_cache, &helper);
 	if (!res) {
 		res = mono_mem_manager_alloc0 (mm, sizeof (MonoInflatedMethodSignature));
 		// FIXME: sig is an inflated signature not owned by the mem manager

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

@@ -2736,7 +2736,7 @@ interp_handle_intrinsics (TransformData *td, MonoMethod *target_method, MonoClas
 				default: return FALSE;
 			}
 		}
-	} 
+	}
 
 	return FALSE;
 }
@@ -10036,8 +10036,7 @@ mono_interp_transform_method (InterpMethod *imethod, ThreadContext *context, Mon
 
 		// FIXME Publishing of seq points seems to be racy with tiereing. We can have both tiered and untiered method
 		// running at the same time. We could therefore get the optimized imethod seq points for the unoptimized method.
-		gpointer seq_points = NULL;
-		dn_simdhash_ght_try_get_value (jit_mm->seq_points, imethod->method, (void **)&seq_points);
+		gpointer seq_points = dn_simdhash_ght_get_value_or_default (jit_mm->seq_points, imethod->method);
 		if (!seq_points || seq_points != imethod->jinfo->seq_points)
 			dn_simdhash_ght_replace (jit_mm->seq_points, imethod->method, imethod->jinfo->seq_points);
 	}

src/mono/mono/mini/seq-points.c

@@ -272,12 +272,12 @@ mono_get_seq_points (MonoMethod *method)
 	// FIXME:
 	jit_mm = get_default_jit_mm ();
 	jit_mm_lock (jit_mm);
-	dn_simdhash_ght_try_get_value (jit_mm->seq_points, method, (void **)&seq_points);
+	seq_points = dn_simdhash_ght_get_value_or_default (jit_mm->seq_points, method);
 	if (!seq_points && method->is_inflated) {
 		/* generic sharing + aot */
-		dn_simdhash_ght_try_get_value (jit_mm->seq_points, declaring_generic_method, (void **)&seq_points);
+		seq_points = dn_simdhash_ght_get_value_or_default (jit_mm->seq_points, declaring_generic_method);
 		if (!seq_points)
-			dn_simdhash_ght_try_get_value (jit_mm->seq_points, shared_method, (void **)&seq_points);
+			seq_points = dn_simdhash_ght_get_value_or_default (jit_mm->seq_points, shared_method);
 	}
 	jit_mm_unlock (jit_mm);
 

src/native/containers/dn-simdhash-ght-compatible.c

@@ -16,27 +16,6 @@ typedef struct dn_simdhash_ght_data {
 	dn_simdhash_ght_destroy_func value_destroy_func;
 } dn_simdhash_ght_data;
 
-static inline uint32_t
-dn_simdhash_ght_hash (dn_simdhash_ght_data data, void * key)
-{
-	dn_simdhash_ght_hash_func hash_func = data.hash_func;
-	if (hash_func)
-		return (uint32_t)hash_func(key);
-	else
-		// FIXME: Seed
-		return MurmurHash3_32_ptr(key, 0);
-}
-
-static inline int32_t
-dn_simdhash_ght_equals (dn_simdhash_ght_data data, void * lhs, void * rhs)
-{
-	dn_simdhash_ght_equal_func equal_func = data.key_equal_func;
-	if (equal_func)
-		return equal_func(lhs, rhs);
-	else
-		return lhs == rhs;
-}
-
 static inline void
 dn_simdhash_ght_removed (dn_simdhash_ght_data data, void * key, void * value)
 {
@@ -68,26 +47,53 @@ dn_simdhash_ght_replaced (dn_simdhash_ght_data data, void * old_key, void * new_
 #define DN_SIMDHASH_KEY_T void *
 #define DN_SIMDHASH_VALUE_T void *
 #define DN_SIMDHASH_INSTANCE_DATA_T dn_simdhash_ght_data
-#define DN_SIMDHASH_KEY_HASHER dn_simdhash_ght_hash
-#define DN_SIMDHASH_KEY_EQUALS dn_simdhash_ght_equals
+
+#define DN_SIMDHASH_SCAN_DATA_T dn_simdhash_ght_equal_func
+#define DN_SIMDHASH_GET_SCAN_DATA(data) data.key_equal_func
+
+#define DN_SIMDHASH_KEY_HASHER(data, key) (uint32_t)data.hash_func(key)
+#define DN_SIMDHASH_KEY_EQUALS(scan_data, lhs, rhs) scan_data(lhs, rhs)
+
 #define DN_SIMDHASH_ON_REMOVE dn_simdhash_ght_removed
 #define DN_SIMDHASH_ON_REPLACE dn_simdhash_ght_replaced
+// perfect cache alignment. 128-byte buckets for 64-bit pointers, 64-byte buckets for 32-bit pointers
 #if SIZEOF_VOID_P == 8
-#define DN_SIMDHASH_BUCKET_CAPACITY 11
+#define DN_SIMDHASH_BUCKET_CAPACITY 14
 #else
 #define DN_SIMDHASH_BUCKET_CAPACITY 12
 #endif
 #define DN_SIMDHASH_NO_DEFAULT_NEW 1
 
 #include "dn-simdhash-specialization.h"
 
+static unsigned int
+dn_simdhash_ght_default_hash (const void * key)
+{
+    // You might think we should avalanche the key bits but in my testing, it doesn't help.
+    // Right now the default hash function is rarely used anyway
+    return (unsigned int)(size_t)key;
+}
+
+static int32_t
+dn_simdhash_ght_default_comparer (const void * a, const void * b)
+{
+	return a == b;
+}
+
 dn_simdhash_ght_t *
 dn_simdhash_ght_new (
 	dn_simdhash_ght_hash_func hash_func, dn_simdhash_ght_equal_func key_equal_func,
 	uint32_t capacity, dn_allocator_t *allocator
 )
 {
 	dn_simdhash_ght_t *hash = dn_simdhash_new_internal(&DN_SIMDHASH_T_META, DN_SIMDHASH_T_VTABLE, capacity, allocator);
+	// Most users of dn_simdhash_ght are passing a custom comparer, and always doing an indirect call ends up being faster
+	//  than conditionally doing a fast inline check when there's no comparer set. Somewhat counter-intuitive, but true
+	//  on both x64 and arm64. Probably due to the smaller code size and reduced branch predictor pressure.
+	if (!hash_func)
+		hash_func = dn_simdhash_ght_default_hash;
+	if (!key_equal_func)
+		key_equal_func = dn_simdhash_ght_default_comparer;
 	dn_simdhash_instance_data(dn_simdhash_ght_data, hash).hash_func = hash_func;
 	dn_simdhash_instance_data(dn_simdhash_ght_data, hash).key_equal_func = key_equal_func;
 	return hash;
@@ -101,6 +107,10 @@ dn_simdhash_ght_new_full (
 )
 {
 	dn_simdhash_ght_t *hash = dn_simdhash_new_internal(&DN_SIMDHASH_T_META, DN_SIMDHASH_T_VTABLE, capacity, allocator);
+	if (!hash_func)
+		hash_func = dn_simdhash_ght_default_hash;
+	if (!key_equal_func)
+		key_equal_func = dn_simdhash_ght_default_comparer;
 	dn_simdhash_instance_data(dn_simdhash_ght_data, hash).hash_func = hash_func;
 	dn_simdhash_instance_data(dn_simdhash_ght_data, hash).key_equal_func = key_equal_func;
 	dn_simdhash_instance_data(dn_simdhash_ght_data, hash).key_destroy_func = key_destroy_func;
@@ -145,3 +155,17 @@ dn_simdhash_ght_insert_replace (
 			return;
 	}
 }
+
+void *
+dn_simdhash_ght_get_value_or_default (
+	dn_simdhash_ght_t *hash, void * key
+)
+{
+	check_self(hash);
+	uint32_t key_hash = DN_SIMDHASH_KEY_HASHER(DN_SIMDHASH_GET_DATA(hash), key);
+	DN_SIMDHASH_VALUE_T *value_ptr = DN_SIMDHASH_FIND_VALUE_INTERNAL(hash, key, key_hash);
+	if (value_ptr)
+		return *value_ptr;
+	else
+		return NULL;
+}

src/native/containers/dn-simdhash-ght-compatible.h

@@ -28,6 +28,12 @@ dn_simdhash_ght_insert_replace (
 	int32_t overwrite_key
 );
 
+// faster and simpler to use than dn_simdhash_ght_try_get_value, use it wisely
+void *
+dn_simdhash_ght_get_value_or_default (
+	dn_simdhash_ght_t *hash, void * key
+);
+
 // compatibility shims for the g_hash_table_ versions in glib.h
 #define dn_simdhash_ght_insert(h,k,v)  dn_simdhash_ght_insert_replace ((h),(k),(v),FALSE)
 #define dn_simdhash_ght_replace(h,k,v) dn_simdhash_ght_insert_replace ((h),(k),(v),TRUE)

src/native/containers/dn-simdhash-ptr-ptr.c

@@ -11,10 +11,11 @@
 #define DN_SIMDHASH_T dn_simdhash_ptr_ptr
 #define DN_SIMDHASH_KEY_T void *
 #define DN_SIMDHASH_VALUE_T void *
-#define DN_SIMDHASH_KEY_HASHER(hash, key) (MurmurHash3_32_ptr(key, 0))
-#define DN_SIMDHASH_KEY_EQUALS(hash, lhs, rhs) (lhs == rhs)
+#define DN_SIMDHASH_KEY_HASHER(data, key) (MurmurHash3_32_ptr(key, 0))
+#define DN_SIMDHASH_KEY_EQUALS(data, lhs, rhs) (lhs == rhs)
+// perfect cache alignment. 128-byte buckets for 64-bit pointers, 64-byte buckets for 32-bit pointers
 #if SIZEOF_VOID_P == 8
-#define DN_SIMDHASH_BUCKET_CAPACITY 11
+#define DN_SIMDHASH_BUCKET_CAPACITY 14
 #else
 #define DN_SIMDHASH_BUCKET_CAPACITY 12
 #endif

src/native/containers/dn-simdhash-ptrpair-ptr.c

@@ -26,8 +26,8 @@ dn_ptrpair_t_equals (dn_ptrpair_t lhs, dn_ptrpair_t rhs)
 #define DN_SIMDHASH_T dn_simdhash_ptrpair_ptr
 #define DN_SIMDHASH_KEY_T dn_ptrpair_t
 #define DN_SIMDHASH_VALUE_T void *
-#define DN_SIMDHASH_KEY_HASHER(hash, key) dn_ptrpair_t_hash(key)
-#define DN_SIMDHASH_KEY_EQUALS(hash, lhs, rhs) dn_ptrpair_t_equals(lhs, rhs)
+#define DN_SIMDHASH_KEY_HASHER(data, key) dn_ptrpair_t_hash(key)
+#define DN_SIMDHASH_KEY_EQUALS(data, lhs, rhs) dn_ptrpair_t_equals(lhs, rhs)
 #if SIZEOF_VOID_P == 8
 // 192 bytes holds 12 16-byte blocks, so 11 keys and one suffix table
 #define DN_SIMDHASH_BUCKET_CAPACITY 11

src/native/containers/dn-simdhash-specialization.h

@@ -23,6 +23,13 @@
 #error Expected DN_SIMDHASH_VALUE_T definition i.e. int
 #endif
 
+// If specified, this data will be precalculated/prefetched at the start of scans
+//  and passed to your KEY_EQUALS macro as its first parameter
+#ifndef DN_SIMDHASH_SCAN_DATA_T
+#define DN_SIMDHASH_SCAN_DATA_T uint8_t
+#define DN_SIMDHASH_GET_SCAN_DATA(data) 0
+#endif
+
 // If specified, we pass instance data to the handlers by-value, otherwise we
 //  pass the pointer to the hash itself by-value. This is enough to allow clang
 //  to hoist the load of the instance data out of the key scan loop, though it
@@ -204,11 +211,13 @@ DN_SIMDHASH_SCAN_BUCKET_INTERNAL (DN_SIMDHASH_T_PTR hash, bucket_t *restrict buc
 #undef bucket_suffixes
 
 	if (DN_LIKELY(index < count)) {
+		DN_SIMDHASH_SCAN_DATA_T scan_data = DN_SIMDHASH_GET_SCAN_DATA(DN_SIMDHASH_GET_DATA(hash));
+		// HACK: Suppress unused variable warning for specializations that don't use scan_data
+		(void)(scan_data);
+
 		DN_SIMDHASH_KEY_T *key = &bucket->keys[index];
 		do {
-			// FIXME: Could be profitable to manually hoist the data load outside of the loop,
-			//  if not out of SCAN_BUCKET_INTERNAL entirely. Clang appears to do LICM on it.
-			if (DN_SIMDHASH_KEY_EQUALS(DN_SIMDHASH_GET_DATA(hash), needle, *key))
+			if (DN_SIMDHASH_KEY_EQUALS(scan_data, needle, *key))
 				return index;
 			key++;
 			index++;