From 28dd8a0d60ba6173dafe3fd46a7ba606aeef3722 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Thu, 26 Jun 2025 09:33:57 -0700 Subject: [PATCH 1/3] Make simdhash add operations return a result enum instead of a 0/1 so they can signal OOM and whether an overwrite happened Make simdhash handle malloc failures during new and resize operations Fix some zeros where NULL would do Move side effect out of assert Explicitly NOMEM if the simdhash add fails due to an out of memory condition Add dn_simdhash_ght_try_insert Cleanup some erroneous uses of assert() instead of dn_simdhash_assert() Address PR feedback Cleanup simdhash failure handling Cleanup Fix CI warnings --- src/coreclr/interpreter/compiler.h | 13 +++- src/coreclr/interpreter/stackmap.cpp | 9 ++- .../containers/dn-simdhash-ght-compatible.c | 61 +++++++++++++++++-- .../containers/dn-simdhash-ght-compatible.h | 7 +++ .../dn-simdhash-specialization-declarations.h | 4 +- .../containers/dn-simdhash-specialization.h | 32 ++++------ .../containers/dn-simdhash-specializations.h | 2 +- .../containers/dn-simdhash-string-ptr.c | 4 +- src/native/containers/dn-simdhash.c | 54 +++++++++++----- src/native/containers/dn-simdhash.h | 29 ++++++++- .../simdhash-benchmark/run-benchmark.ps1 | 2 +- 11 files changed, 167 insertions(+), 50 deletions(-) diff --git a/src/coreclr/interpreter/compiler.h b/src/coreclr/interpreter/compiler.h index 6a534f1152c0dd..9f48473892f5ee 100644 --- a/src/coreclr/interpreter/compiler.h +++ b/src/coreclr/interpreter/compiler.h @@ -548,7 +548,9 @@ class InterpCompiler } void AddPointerToNameMap(void* ptr, const char* name) { - dn_simdhash_ptr_ptr_try_add(m_pointerToNameMap.GetValue(), ptr, (void*)name); + dn_simdhash_add_result addResult = dn_simdhash_ptr_ptr_try_add(m_pointerToNameMap.GetValue(), ptr, (void*)name); + if (addResult < 0) + NOMEM(); } void PrintNameInPointerMap(void* ptr); #endif @@ -865,7 +867,14 @@ int32_t InterpDataItemIndexMap::GetDataItemIndexForT(const T& lookup) VarSizedDataWithPayload* pLookup = new(hashItemPayload) VarSizedDataWithPayload(); memcpy(&pLookup->payload, &lookup, sizeof(T)); - dn_simdhash_ght_insert(hash, (void*)pLookup, (void*)(size_t)dataItemIndex); + dn_simdhash_add_result addResult = dn_simdhash_ght_try_insert( + hash, (void*)pLookup, (void*)(size_t)dataItemIndex, DN_SIMDHASH_INSERT_MODE_ENSURE_UNIQUE + ); + if (addResult == DN_SIMDHASH_OUT_OF_MEMORY) + NOMEM(); + else if (addResult != DN_SIMDHASH_ADD_INSERTED) + assert(!"Internal error in dn_simdhash"); + return dataItemIndex; } diff --git a/src/coreclr/interpreter/stackmap.cpp b/src/coreclr/interpreter/stackmap.cpp index 340b5dbb7fce76..06f789e03d96fa 100644 --- a/src/coreclr/interpreter/stackmap.cpp +++ b/src/coreclr/interpreter/stackmap.cpp @@ -24,10 +24,17 @@ InterpreterStackMap* GetInterpreterStackMap(ICorJitInfo* jitInfo, CORINFO_CLASS_ InterpreterStackMap* result = nullptr; if (!t_sharedStackMapLookup) t_sharedStackMapLookup = dn_simdhash_ptr_ptr_new(0, nullptr); + if (!t_sharedStackMapLookup) + NOMEM(); + if (!dn_simdhash_ptr_ptr_try_get_value(t_sharedStackMapLookup, classHandle, (void **)&result)) { result = new InterpreterStackMap(jitInfo, classHandle); - dn_simdhash_ptr_ptr_try_add(t_sharedStackMapLookup, classHandle, result); + dn_simdhash_add_result addResult = dn_simdhash_ptr_ptr_try_add(t_sharedStackMapLookup, classHandle, result); + if (addResult == DN_SIMDHASH_OUT_OF_MEMORY) + NOMEM(); + else + assert(addResult == DN_SIMDHASH_ADD_INSERTED); } return result; } diff --git a/src/native/containers/dn-simdhash-ght-compatible.c b/src/native/containers/dn-simdhash-ght-compatible.c index 41b6f3d5403430..1a3cd566f55c1e 100644 --- a/src/native/containers/dn-simdhash-ght-compatible.c +++ b/src/native/containers/dn-simdhash-ght-compatible.c @@ -69,9 +69,9 @@ dn_simdhash_ght_replaced (dn_simdhash_ght_data data, void * old_key, void * new_ 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; + // 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 @@ -87,6 +87,8 @@ dn_simdhash_ght_new ( ) { dn_simdhash_ght_t *hash = dn_simdhash_new_internal(&DN_SIMDHASH_T_META, DN_SIMDHASH_T_VTABLE, capacity, allocator); + if (!hash) + return NULL; // 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. @@ -107,6 +109,8 @@ 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) + return NULL; if (!hash_func) hash_func = dn_simdhash_ght_default_hash; if (!key_equal_func) @@ -133,7 +137,14 @@ dn_simdhash_ght_insert_replace ( dn_simdhash_insert_result ok = DN_SIMDHASH_TRY_INSERT_INTERNAL(hash, key, key_hash, value, imode); if (ok == DN_SIMDHASH_INSERT_NEED_TO_GROW) { - dn_simdhash_buffers_t old_buffers = dn_simdhash_ensure_capacity_internal(hash, dn_simdhash_capacity(hash) + 1); + uint8_t grow_ok; + dn_simdhash_buffers_t old_buffers = dn_simdhash_ensure_capacity_internal(hash, dn_simdhash_capacity(hash) + 1, &grow_ok); + // The ghashtable API doesn't have a way to signal failure, so just assert that we didn't fail to grow + dn_simdhash_assert(grow_ok); + // Don't attempt to insert after a failed grow (it's possible to get here because dn_simdhash_assert is configurable) + if (!grow_ok) + return; + if (old_buffers.buckets) { DN_SIMDHASH_REHASH_INTERNAL(hash, old_buffers); dn_simdhash_free_buffers(old_buffers); @@ -151,11 +162,51 @@ dn_simdhash_ght_insert_replace ( case DN_SIMDHASH_INSERT_KEY_ALREADY_PRESENT: case DN_SIMDHASH_INSERT_NEED_TO_GROW: default: - assert(0); + dn_simdhash_assert(!"Internal error in dn_simdhash_ght_insert_replace"); return; } } +dn_simdhash_add_result +dn_simdhash_ght_try_insert ( + dn_simdhash_ght_t *hash, + void *key, void *value, + dn_simdhash_insert_mode mode +) +{ + check_self(hash); + uint32_t key_hash = DN_SIMDHASH_KEY_HASHER(DN_SIMDHASH_GET_DATA(hash), key); + + dn_simdhash_insert_result ok = DN_SIMDHASH_TRY_INSERT_INTERNAL(hash, key, key_hash, value, mode); + if (ok == DN_SIMDHASH_INSERT_NEED_TO_GROW) { + uint8_t grow_ok; + dn_simdhash_buffers_t old_buffers = dn_simdhash_ensure_capacity_internal(hash, dn_simdhash_capacity(hash) + 1, &grow_ok); + if (!grow_ok) + return DN_SIMDHASH_OUT_OF_MEMORY; + + if (old_buffers.buckets) { + DN_SIMDHASH_REHASH_INTERNAL(hash, old_buffers); + dn_simdhash_free_buffers(old_buffers); + } + ok = DN_SIMDHASH_TRY_INSERT_INTERNAL(hash, key, key_hash, value, mode); + } + + switch (ok) { + case DN_SIMDHASH_INSERT_OK_ADDED_NEW: + hash->count++; + return DN_SIMDHASH_ADD_INSERTED; + case DN_SIMDHASH_INSERT_OK_OVERWROTE_EXISTING: + return DN_SIMDHASH_ADD_OVERWROTE; + case DN_SIMDHASH_INSERT_KEY_ALREADY_PRESENT: + return DN_SIMDHASH_ADD_FAILED; + + case DN_SIMDHASH_INSERT_NEED_TO_GROW: + default: + dn_simdhash_assert(!"Internal error in dn_simdhash_ght_insert_replace"); + return DN_SIMDHASH_INTERNAL_ERROR; + } +} + void * dn_simdhash_ght_get_value_or_default ( dn_simdhash_ght_t *hash, void * key diff --git a/src/native/containers/dn-simdhash-ght-compatible.h b/src/native/containers/dn-simdhash-ght-compatible.h index 76f25ae09946c1..c11147bd35c2b4 100644 --- a/src/native/containers/dn-simdhash-ght-compatible.h +++ b/src/native/containers/dn-simdhash-ght-compatible.h @@ -38,6 +38,13 @@ dn_simdhash_ght_get_value_or_default ( dn_simdhash_ght_t *hash, void * key ); +dn_simdhash_add_result +dn_simdhash_ght_try_insert ( + dn_simdhash_ght_t *hash, + void *key, void *value, + dn_simdhash_insert_mode mode +); + #ifdef __cplusplus } #endif // __cplusplus diff --git a/src/native/containers/dn-simdhash-specialization-declarations.h b/src/native/containers/dn-simdhash-specialization-declarations.h index 97dc6fe32d2310..bde49893337d29 100644 --- a/src/native/containers/dn-simdhash-specialization-declarations.h +++ b/src/native/containers/dn-simdhash-specialization-declarations.h @@ -52,10 +52,10 @@ DN_SIMDHASH_T_PTR DN_SIMDHASH_NEW (uint32_t capacity, dn_allocator_t *allocator); #endif -uint8_t +dn_simdhash_add_result DN_SIMDHASH_TRY_ADD (DN_SIMDHASH_T_PTR hash, DN_SIMDHASH_KEY_T key, DN_SIMDHASH_VALUE_T value); -uint8_t +dn_simdhash_add_result DN_SIMDHASH_TRY_ADD_WITH_HASH (DN_SIMDHASH_T_PTR hash, DN_SIMDHASH_KEY_T key, uint32_t key_hash, DN_SIMDHASH_VALUE_T value); // result is an optional parameter that will be set to the value of the item if it was found. diff --git a/src/native/containers/dn-simdhash-specialization.h b/src/native/containers/dn-simdhash-specialization.h index 7614a690f5c080..408a70a713ee26 100644 --- a/src/native/containers/dn-simdhash-specialization.h +++ b/src/native/containers/dn-simdhash-specialization.h @@ -305,17 +305,6 @@ DN_SIMDHASH_FIND_VALUE_INTERNAL (DN_SIMDHASH_T_PTR hash, DN_SIMDHASH_KEY_T key, return NULL; } -typedef enum dn_simdhash_insert_mode { - // Ensures that no matching key exists in the hash, then adds the key/value pair - DN_SIMDHASH_INSERT_MODE_ENSURE_UNIQUE, - // If a matching key exists in the hash, overwrite its value but leave the key alone - DN_SIMDHASH_INSERT_MODE_OVERWRITE_VALUE, - // If a matching key exists in the hash, overwrite both the key and the value - DN_SIMDHASH_INSERT_MODE_OVERWRITE_KEY_AND_VALUE, - // Do not scan for existing matches before adding the new key/value pair. - DN_SIMDHASH_INSERT_MODE_REHASHING, -} dn_simdhash_insert_mode; - static void do_overwrite ( DN_SIMDHASH_T_PTR hash, uint32_t bucket_index, bucket_t *bucket_address, int index_in_bucket, @@ -444,7 +433,7 @@ DN_SIMDHASH_NEW (uint32_t capacity, dn_allocator_t *allocator) } #endif -uint8_t +dn_simdhash_add_result DN_SIMDHASH_TRY_ADD (DN_SIMDHASH_T_PTR hash, DN_SIMDHASH_KEY_T key, DN_SIMDHASH_VALUE_T value) { check_self(hash); @@ -453,14 +442,18 @@ DN_SIMDHASH_TRY_ADD (DN_SIMDHASH_T_PTR hash, DN_SIMDHASH_KEY_T key, DN_SIMDHASH_ return DN_SIMDHASH_TRY_ADD_WITH_HASH(hash, key, key_hash, value); } -uint8_t +dn_simdhash_add_result DN_SIMDHASH_TRY_ADD_WITH_HASH (DN_SIMDHASH_T_PTR hash, DN_SIMDHASH_KEY_T key, uint32_t key_hash, DN_SIMDHASH_VALUE_T value) { check_self(hash); dn_simdhash_insert_result ok = DN_SIMDHASH_TRY_INSERT_INTERNAL(hash, key, key_hash, value, DN_SIMDHASH_INSERT_MODE_ENSURE_UNIQUE); if (ok == DN_SIMDHASH_INSERT_NEED_TO_GROW) { - dn_simdhash_buffers_t old_buffers = dn_simdhash_ensure_capacity_internal(hash, dn_simdhash_capacity(hash) + 1); + uint8_t grow_ok; + dn_simdhash_buffers_t old_buffers = dn_simdhash_ensure_capacity_internal(hash, dn_simdhash_capacity(hash) + 1, &grow_ok); + if (!grow_ok) + return DN_SIMDHASH_OUT_OF_MEMORY; + if (old_buffers.buckets) { DN_SIMDHASH_REHASH_INTERNAL(hash, old_buffers); dn_simdhash_free_buffers(old_buffers); @@ -471,18 +464,19 @@ DN_SIMDHASH_TRY_ADD_WITH_HASH (DN_SIMDHASH_T_PTR hash, DN_SIMDHASH_KEY_T key, ui switch (ok) { case DN_SIMDHASH_INSERT_OK_ADDED_NEW: hash->count++; - return 1; + return DN_SIMDHASH_ADD_INSERTED; case DN_SIMDHASH_INSERT_OK_OVERWROTE_EXISTING: // This shouldn't happen dn_simdhash_assert(!"Overwrote an existing item while adding"); - return 1; + return DN_SIMDHASH_INTERNAL_ERROR; case DN_SIMDHASH_INSERT_KEY_ALREADY_PRESENT: - return 0; + return DN_SIMDHASH_ADD_FAILED; case DN_SIMDHASH_INSERT_NEED_TO_GROW: - // We should always have enough space after growing once. + dn_simdhash_assert(!"After growing there was no space for a new item"); + return DN_SIMDHASH_INTERNAL_ERROR; default: dn_simdhash_assert(!"Failed to add a new item but there was no existing item"); - return 0; + return DN_SIMDHASH_INTERNAL_ERROR; } } diff --git a/src/native/containers/dn-simdhash-specializations.h b/src/native/containers/dn-simdhash-specializations.h index 9533edfc5f3d1f..e899b9ed38270e 100644 --- a/src/native/containers/dn-simdhash-specializations.h +++ b/src/native/containers/dn-simdhash-specializations.h @@ -61,7 +61,7 @@ typedef struct dn_simdhash_str_key dn_simdhash_str_key; typedef struct dn_ptrpair_t { - void *first, *second; + void *first, *second; } dn_ptrpair_t; #define DN_SIMDHASH_T dn_simdhash_ptrpair_ptr diff --git a/src/native/containers/dn-simdhash-string-ptr.c b/src/native/containers/dn-simdhash-string-ptr.c index c3a93ceac9cdba..d917f3386c440e 100644 --- a/src/native/containers/dn-simdhash-string-ptr.c +++ b/src/native/containers/dn-simdhash-string-ptr.c @@ -83,8 +83,8 @@ dn_simdhash_string_ptr_try_remove (dn_simdhash_string_ptr_t *hash, const char *k void dn_simdhash_string_ptr_foreach (dn_simdhash_string_ptr_t *hash, dn_simdhash_string_ptr_foreach_func func, void *user_data) { - assert(hash); - assert(func); + dn_simdhash_assert(hash); + dn_simdhash_assert(func); dn_simdhash_buffers_t buffers = hash->buffers; BEGIN_SCAN_PAIRS(buffers, key_address, value_address) diff --git a/src/native/containers/dn-simdhash.c b/src/native/containers/dn-simdhash.c index 24ea919f8ae365..f29d42b81e9ca8 100644 --- a/src/native/containers/dn-simdhash.c +++ b/src/native/containers/dn-simdhash.c @@ -167,7 +167,9 @@ dn_simdhash_new_internal (dn_simdhash_meta_t *meta, dn_simdhash_vtable_t vtable, { const size_t size = sizeof(dn_simdhash_t) + meta->data_size; dn_simdhash_t *result = (dn_simdhash_t *)dn_allocator_alloc(allocator, size); - dn_simdhash_assert(result); + if (!result) + return NULL; + memset(result, 0, size); dn_simdhash_assert(meta); @@ -178,7 +180,12 @@ dn_simdhash_new_internal (dn_simdhash_meta_t *meta, dn_simdhash_vtable_t vtable, result->vtable = vtable; result->buffers.allocator = allocator; - dn_simdhash_ensure_capacity_internal(result, compute_adjusted_capacity(capacity)); + uint8_t alloc_ok; + dn_simdhash_ensure_capacity_internal(result, compute_adjusted_capacity(capacity), &alloc_ok); + if (!alloc_ok) { + dn_allocator_free(allocator, result); + return NULL; + } return result; } @@ -205,9 +212,12 @@ dn_simdhash_free_buffers (dn_simdhash_buffers_t buffers) } dn_simdhash_buffers_t -dn_simdhash_ensure_capacity_internal (dn_simdhash_t *hash, uint32_t capacity) +dn_simdhash_ensure_capacity_internal (dn_simdhash_t *hash, uint32_t capacity, uint8_t *ok) { dn_simdhash_assert(hash); + dn_simdhash_assert(ok); + *ok = 0; + size_t bucket_count = (capacity + hash->meta->bucket_capacity - 1) / hash->meta->bucket_capacity; // FIXME: Only apply this when capacity == 0? if (bucket_count < DN_SIMDHASH_MIN_BUCKET_COUNT) @@ -225,6 +235,8 @@ dn_simdhash_ensure_capacity_internal (dn_simdhash_t *hash, uint32_t capacity) dn_simdhash_buffers_t result = { 0, }; if (bucket_count <= hash->buffers.buckets_length) { dn_simdhash_assert(value_count <= hash->buffers.values_length); + // We didn't grow but we also didn't fail, so we set ok to 1. + *ok = 1; return result; } @@ -235,9 +247,26 @@ dn_simdhash_ensure_capacity_internal (dn_simdhash_t *hash, uint32_t capacity) capacity, value_count ); */ + + // pad buckets allocation by the width of one vector so we can align it + size_t buckets_size_bytes = (bucket_count * hash->meta->bucket_size_bytes) + DN_SIMDHASH_VECTOR_WIDTH, + values_size_bytes = value_count * hash->meta->value_size; + + // If either of these allocations fail all we can do is return a default-initialized buffers_t, which will + // result in the caller not freeing anything and seeing an ok of 0, so they can tell the grow failed. + // This should leave the hash in a well-formed state and if they try to grow later it might work. + void *new_buckets = dn_allocator_alloc(hash->buffers.allocator, buckets_size_bytes); + if (!new_buckets) + return result; + + void *new_values = dn_allocator_alloc(hash->buffers.allocator, values_size_bytes); + if (!new_values) { + dn_allocator_free(hash->buffers.allocator, new_buckets); + return result; + } + // Store old buffers so caller can rehash and then free them result = hash->buffers; - size_t grow_at_count = value_count; grow_at_count *= 100; grow_at_count /= DN_SIMDHASH_SIZING_PERCENTAGE; @@ -245,13 +274,6 @@ dn_simdhash_ensure_capacity_internal (dn_simdhash_t *hash, uint32_t capacity) hash->buffers.buckets_length = (uint32_t)bucket_count; hash->buffers.values_length = (uint32_t)value_count; - // pad buckets allocation by the width of one vector so we can align it - size_t buckets_size_bytes = (bucket_count * hash->meta->bucket_size_bytes) + DN_SIMDHASH_VECTOR_WIDTH, - values_size_bytes = value_count * hash->meta->value_size; - - void *new_buckets = dn_allocator_alloc(hash->buffers.allocator, buckets_size_bytes), - *new_values = dn_allocator_alloc(hash->buffers.allocator, values_size_bytes); - dn_simdhash_assert(new_buckets); dn_simdhash_assert(new_values); @@ -268,6 +290,8 @@ dn_simdhash_ensure_capacity_internal (dn_simdhash_t *hash, uint32_t capacity) // Skip this for performance; memset is especially slow in wasm // memset(hash->buffers.values, 0, values_size_bytes); + *ok = 1; + return result; } @@ -301,7 +325,7 @@ dn_simdhash_count (dn_simdhash_t *hash) uint32_t dn_simdhash_overflow_count (dn_simdhash_t *hash) { - assert(hash); + dn_simdhash_assert(hash); uint32_t result = 0; for (uint32_t bucket_index = 0; bucket_index < hash->buffers.buckets_length; bucket_index++) { uint8_t *suffixes = ((uint8_t *)hash->buffers.buckets) + (bucket_index * hash->meta->bucket_size_bytes); @@ -311,14 +335,16 @@ dn_simdhash_overflow_count (dn_simdhash_t *hash) return result; } -void +uint8_t dn_simdhash_ensure_capacity (dn_simdhash_t *hash, uint32_t capacity) { dn_simdhash_assert(hash); capacity = compute_adjusted_capacity(capacity); - dn_simdhash_buffers_t old_buffers = dn_simdhash_ensure_capacity_internal(hash, capacity); + uint8_t result; + dn_simdhash_buffers_t old_buffers = dn_simdhash_ensure_capacity_internal(hash, capacity, &result); if (old_buffers.buckets) { hash->vtable.rehash(hash, old_buffers); dn_simdhash_free_buffers(old_buffers); } + return result; } diff --git a/src/native/containers/dn-simdhash.h b/src/native/containers/dn-simdhash.h index af1afd8a910c7f..13bcb9ec828e4b 100644 --- a/src/native/containers/dn-simdhash.h +++ b/src/native/containers/dn-simdhash.h @@ -52,11 +52,32 @@ typedef struct dn_simdhash_t dn_simdhash_t; typedef struct dn_simdhash_meta_t { // type metadata for generic implementation + // NOTE: key_size and value_size are not used consistently by every part of the implementation, + // a specialization is still strongly typed based on its KEY_T and VALUE_T. But they need to match. uint32_t bucket_capacity, bucket_size_bytes, key_size, value_size, // Allocate this many bytes of extra data inside the dn_simdhash_t data_size; } dn_simdhash_meta_t; +typedef enum dn_simdhash_insert_mode { + // Ensures that no matching key exists in the hash, then adds the key/value pair + DN_SIMDHASH_INSERT_MODE_ENSURE_UNIQUE, + // If a matching key exists in the hash, overwrite its value but leave the key alone + DN_SIMDHASH_INSERT_MODE_OVERWRITE_VALUE, + // If a matching key exists in the hash, overwrite both the key and the value + DN_SIMDHASH_INSERT_MODE_OVERWRITE_KEY_AND_VALUE, + // Do not scan for existing matches before adding the new key/value pair. + DN_SIMDHASH_INSERT_MODE_REHASHING, +} dn_simdhash_insert_mode; + +typedef enum dn_simdhash_add_result { + DN_SIMDHASH_INTERNAL_ERROR = -2, + DN_SIMDHASH_OUT_OF_MEMORY = -1, + DN_SIMDHASH_ADD_FAILED = 0, + DN_SIMDHASH_ADD_INSERTED = 1, + DN_SIMDHASH_ADD_OVERWROTE = 2, +} dn_simdhash_add_result; + typedef enum dn_simdhash_insert_result { DN_SIMDHASH_INSERT_OK_ADDED_NEW, DN_SIMDHASH_INSERT_OK_OVERWROTE_EXISTING, @@ -139,8 +160,9 @@ dn_simdhash_free_buffers (dn_simdhash_buffers_t buffers); // If a resize happens, this will allocate new buffers and return the old ones. // It is your responsibility to rehash and then free the old buffers. +// If growing failed due to an out of memory condition *ok will be 0, otherwise it will be 1. dn_simdhash_buffers_t -dn_simdhash_ensure_capacity_internal (dn_simdhash_t *hash, uint32_t capacity); +dn_simdhash_ensure_capacity_internal (dn_simdhash_t *hash, uint32_t capacity, uint8_t *ok); // Erases the contents of the table, but does not shrink it. void @@ -161,8 +183,9 @@ uint32_t dn_simdhash_overflow_count (dn_simdhash_t *hash); // Automatically resizes the table if it is too small to hold the requested number -// of items. Will not shrink the table if it is already bigger. -void +// of items. Will not shrink the table if it is already bigger. Returns whether +// the operation was successful - 0 indicates allocation failure. +uint8_t dn_simdhash_ensure_capacity (dn_simdhash_t *hash, uint32_t capacity); #ifdef __cplusplus diff --git a/src/native/containers/simdhash-benchmark/run-benchmark.ps1 b/src/native/containers/simdhash-benchmark/run-benchmark.ps1 index 9417d16c505cbf..1a4221b9ad0b75 100755 --- a/src/native/containers/simdhash-benchmark/run-benchmark.ps1 +++ b/src/native/containers/simdhash-benchmark/run-benchmark.ps1 @@ -1,2 +1,2 @@ -cl /GS- /O2 /std:c17 ./*.c ../dn-simdhash-u32-ptr.c ../dn-simdhash.c ../dn-vector.c ../dn-simdhash-string-ptr.c /DNO_CONFIG_H /DSIZEOF_VOID_P=8 /DNDEBUG /Fe:all-measurements.exe +cl /GS- /O2 /std:c17 ./*.c ../dn-simdhash-u32-ptr.c ../dn-simdhash.c ../dn-vector.c ../dn-simdhash-string-ptr.c ../dn-simdhash-ght-compatible.c /DNO_CONFIG_H /DSIZEOF_VOID_P=8 /DNDEBUG /Fe:all-measurements.exe ./all-measurements.exe From 57a58d6b7cade58e246048e12ca18bac457bc778 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Sat, 28 Jun 2025 13:58:47 -0700 Subject: [PATCH 2/3] simdhash-related cleanups --- src/coreclr/interpreter/compiler.h | 76 +++------------------------ src/coreclr/interpreter/failures.h | 17 ++++++ src/coreclr/interpreter/interpreter.h | 3 +- src/coreclr/interpreter/simdhash.h | 72 +++++++++++++++++++++++++ src/coreclr/interpreter/stackmap.cpp | 13 ++--- 5 files changed, 100 insertions(+), 81 deletions(-) create mode 100644 src/coreclr/interpreter/failures.h create mode 100644 src/coreclr/interpreter/simdhash.h diff --git a/src/coreclr/interpreter/compiler.h b/src/coreclr/interpreter/compiler.h index 9f48473892f5ee..e0df42c95e2349 100644 --- a/src/coreclr/interpreter/compiler.h +++ b/src/coreclr/interpreter/compiler.h @@ -9,52 +9,8 @@ #include "datastructs.h" #include "enum_class_flags.h" #include - -#include "../../native/containers/dn-simdhash.h" -#include "../../native/containers/dn-simdhash-specializations.h" -#include "../../native/containers/dn-simdhash-utils.h" - -class dn_simdhash_ptr_ptr_holder -{ - dn_simdhash_ptr_ptr_t *Value; -public: - dn_simdhash_ptr_ptr_holder() : - Value(nullptr) - { - } - - dn_simdhash_ptr_ptr_t* GetValue() - { - if (!Value) - Value = dn_simdhash_ptr_ptr_new(0, nullptr); - return Value; - } - - dn_simdhash_ptr_ptr_holder(const dn_simdhash_ptr_ptr_holder&) = delete; - dn_simdhash_ptr_ptr_holder& operator=(const dn_simdhash_ptr_ptr_holder&) = delete; - dn_simdhash_ptr_ptr_holder(dn_simdhash_ptr_ptr_holder&& other) - { - Value = other.Value; - other.Value = nullptr; - } - dn_simdhash_ptr_ptr_holder& operator=(dn_simdhash_ptr_ptr_holder&& other) - { - if (this != &other) - { - if (Value != nullptr) - dn_simdhash_free(Value); - Value = other.Value; - other.Value = nullptr; - } - return *this; - } - - ~dn_simdhash_ptr_ptr_holder() - { - if (Value != nullptr) - dn_simdhash_free(Value); - } -}; +#include "failures.h" +#include "simdhash.h" struct InterpException { @@ -67,16 +23,6 @@ struct InterpException const CorJitResult m_result; }; -#if defined(__GNUC__) || defined(__clang__) -#define INTERPRETER_NORETURN __attribute__((noreturn)) -#else -#define INTERPRETER_NORETURN __declspec(noreturn) -#endif - -INTERPRETER_NORETURN void NO_WAY(const char* message); -INTERPRETER_NORETURN void BADCODE(const char* message); -INTERPRETER_NORETURN void NOMEM(); - class InterpCompiler; class InterpDataItemIndexMap @@ -540,17 +486,11 @@ class InterpCompiler dn_simdhash_ptr_ptr_holder m_pointerToNameMap; bool PointerInNameMap(void* ptr) { - if (dn_simdhash_ptr_ptr_try_get_value(m_pointerToNameMap.GetValue(), ptr, NULL)) - { - return true; - } - return false; + return dn_simdhash_ptr_ptr_try_get_value(m_pointerToNameMap.GetValue(), ptr, NULL) != 0; } void AddPointerToNameMap(void* ptr, const char* name) { - dn_simdhash_add_result addResult = dn_simdhash_ptr_ptr_try_add(m_pointerToNameMap.GetValue(), ptr, (void*)name); - if (addResult < 0) - NOMEM(); + assertNoError(dn_simdhash_ptr_ptr_try_add(m_pointerToNameMap.GetValue(), ptr, (void*)name)); } void PrintNameInPointerMap(void* ptr); #endif @@ -867,13 +807,9 @@ int32_t InterpDataItemIndexMap::GetDataItemIndexForT(const T& lookup) VarSizedDataWithPayload* pLookup = new(hashItemPayload) VarSizedDataWithPayload(); memcpy(&pLookup->payload, &lookup, sizeof(T)); - dn_simdhash_add_result addResult = dn_simdhash_ght_try_insert( + assertAddedNew(dn_simdhash_ght_try_insert( hash, (void*)pLookup, (void*)(size_t)dataItemIndex, DN_SIMDHASH_INSERT_MODE_ENSURE_UNIQUE - ); - if (addResult == DN_SIMDHASH_OUT_OF_MEMORY) - NOMEM(); - else if (addResult != DN_SIMDHASH_ADD_INSERTED) - assert(!"Internal error in dn_simdhash"); + )); return dataItemIndex; } diff --git a/src/coreclr/interpreter/failures.h b/src/coreclr/interpreter/failures.h new file mode 100644 index 00000000000000..9532b7bfba0c45 --- /dev/null +++ b/src/coreclr/interpreter/failures.h @@ -0,0 +1,17 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#ifndef _FAILURES_H_ +#define _FAILURES_H_ + +#if defined(__GNUC__) || defined(__clang__) +#define INTERPRETER_NORETURN __attribute__((noreturn)) +#else +#define INTERPRETER_NORETURN __declspec(noreturn) +#endif + +INTERPRETER_NORETURN void NO_WAY(const char* message); +INTERPRETER_NORETURN void BADCODE(const char* message); +INTERPRETER_NORETURN void NOMEM(); + +#endif diff --git a/src/coreclr/interpreter/interpreter.h b/src/coreclr/interpreter/interpreter.h index 90c62b98af6dfe..c1883444ad4344 100644 --- a/src/coreclr/interpreter/interpreter.h +++ b/src/coreclr/interpreter/interpreter.h @@ -25,8 +25,9 @@ #define ALIGN_UP_TO(val,align) ((((size_t)val) + (size_t)((align) - 1)) & (~((size_t)(align - 1)))) -#ifdef DEBUG extern "C" void assertAbort(const char* why, const char* file, unsigned line); + +#ifdef DEBUG #undef assert #define assert(p) (void)((p) || (assertAbort(#p, __FILE__, __LINE__), 0)) #endif // DEBUG diff --git a/src/coreclr/interpreter/simdhash.h b/src/coreclr/interpreter/simdhash.h new file mode 100644 index 00000000000000..b939bc25fd93c2 --- /dev/null +++ b/src/coreclr/interpreter/simdhash.h @@ -0,0 +1,72 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#ifndef _SIMDHASH_H_ +#define _SIMDHASH_H_ + +#include "failures.h" +#include "../../native/containers/dn-simdhash.h" +#include "../../native/containers/dn-simdhash-specializations.h" +#include "../../native/containers/dn-simdhash-utils.h" + +class dn_simdhash_ptr_ptr_holder +{ + dn_simdhash_ptr_ptr_t *Value; +public: + dn_simdhash_ptr_ptr_holder() : + Value(nullptr) + { + } + + dn_simdhash_ptr_ptr_t* GetValue() + { + if (!Value) + Value = dn_simdhash_ptr_ptr_new(0, nullptr); + return Value; + } + + dn_simdhash_ptr_ptr_holder(const dn_simdhash_ptr_ptr_holder&) = delete; + dn_simdhash_ptr_ptr_holder& operator=(const dn_simdhash_ptr_ptr_holder&) = delete; + dn_simdhash_ptr_ptr_holder(dn_simdhash_ptr_ptr_holder&& other) + { + Value = other.Value; + other.Value = nullptr; + } + dn_simdhash_ptr_ptr_holder& operator=(dn_simdhash_ptr_ptr_holder&& other) + { + if (this != &other) + { + if (Value != nullptr) + dn_simdhash_free(Value); + Value = other.Value; + other.Value = nullptr; + } + return *this; + } + + ~dn_simdhash_ptr_ptr_holder() + { + if (Value != nullptr) + dn_simdhash_free(Value); + } +}; + +// Asserts that no error occurred during a simdhash add, but does not mind if no new item was inserted +inline void assertNoError(dn_simdhash_add_result result) +{ + if (result == DN_SIMDHASH_OUT_OF_MEMORY) + NOMEM(); + else if (result < 0) + assert(!"Internal error in simdhash"); +} + +// Asserts that a new item was successfully inserted into the simdhash +inline void assertAddedNew(dn_simdhash_add_result result) +{ + if (result == DN_SIMDHASH_OUT_OF_MEMORY) + NOMEM(); + else if (result != DN_SIMDHASH_ADD_INSERTED) + assert(!"Failed to add new item into simdhash"); +} + +#endif // _SIMDHASH_H_ diff --git a/src/coreclr/interpreter/stackmap.cpp b/src/coreclr/interpreter/stackmap.cpp index 06f789e03d96fa..5e2de5e35ea43f 100644 --- a/src/coreclr/interpreter/stackmap.cpp +++ b/src/coreclr/interpreter/stackmap.cpp @@ -6,11 +6,8 @@ #include "interpreter.h" #include "stackmap.h" -#include "../../native/containers/dn-simdhash.h" -#include "../../native/containers/dn-simdhash-specializations.h" -#include "../../native/containers/dn-simdhash-utils.h" - -extern "C" void assertAbort(const char* why, const char* file, unsigned line); +#include "failures.h" +#include "simdhash.h" void dn_simdhash_assert_fail (const char* file, int line, const char* condition) { @@ -30,11 +27,7 @@ InterpreterStackMap* GetInterpreterStackMap(ICorJitInfo* jitInfo, CORINFO_CLASS_ if (!dn_simdhash_ptr_ptr_try_get_value(t_sharedStackMapLookup, classHandle, (void **)&result)) { result = new InterpreterStackMap(jitInfo, classHandle); - dn_simdhash_add_result addResult = dn_simdhash_ptr_ptr_try_add(t_sharedStackMapLookup, classHandle, result); - if (addResult == DN_SIMDHASH_OUT_OF_MEMORY) - NOMEM(); - else - assert(addResult == DN_SIMDHASH_ADD_INSERTED); + assertAddedNew(dn_simdhash_ptr_ptr_try_add(t_sharedStackMapLookup, classHandle, result)); } return result; } From 708af222f9cec12163bbb6d6227519392aa62437 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 8 Jul 2025 09:35:32 -0700 Subject: [PATCH 3/3] Address PR feedback --- src/coreclr/interpreter/compiler.h | 4 ++-- src/coreclr/interpreter/interpreter.h | 2 +- src/coreclr/interpreter/simdhash.h | 8 ++++---- src/coreclr/interpreter/stackmap.cpp | 6 +++++- 4 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/coreclr/interpreter/compiler.h b/src/coreclr/interpreter/compiler.h index e0df42c95e2349..c483c6fba51d66 100644 --- a/src/coreclr/interpreter/compiler.h +++ b/src/coreclr/interpreter/compiler.h @@ -490,7 +490,7 @@ class InterpCompiler } void AddPointerToNameMap(void* ptr, const char* name) { - assertNoError(dn_simdhash_ptr_ptr_try_add(m_pointerToNameMap.GetValue(), ptr, (void*)name)); + checkNoError(dn_simdhash_ptr_ptr_try_add(m_pointerToNameMap.GetValue(), ptr, (void*)name)); } void PrintNameInPointerMap(void* ptr); #endif @@ -807,7 +807,7 @@ int32_t InterpDataItemIndexMap::GetDataItemIndexForT(const T& lookup) VarSizedDataWithPayload* pLookup = new(hashItemPayload) VarSizedDataWithPayload(); memcpy(&pLookup->payload, &lookup, sizeof(T)); - assertAddedNew(dn_simdhash_ght_try_insert( + checkAddedNew(dn_simdhash_ght_try_insert( hash, (void*)pLookup, (void*)(size_t)dataItemIndex, DN_SIMDHASH_INSERT_MODE_ENSURE_UNIQUE )); diff --git a/src/coreclr/interpreter/interpreter.h b/src/coreclr/interpreter/interpreter.h index c1883444ad4344..969a3c3c73ee0c 100644 --- a/src/coreclr/interpreter/interpreter.h +++ b/src/coreclr/interpreter/interpreter.h @@ -25,9 +25,9 @@ #define ALIGN_UP_TO(val,align) ((((size_t)val) + (size_t)((align) - 1)) & (~((size_t)(align - 1)))) +#ifdef DEBUG extern "C" void assertAbort(const char* why, const char* file, unsigned line); -#ifdef DEBUG #undef assert #define assert(p) (void)((p) || (assertAbort(#p, __FILE__, __LINE__), 0)) #endif // DEBUG diff --git a/src/coreclr/interpreter/simdhash.h b/src/coreclr/interpreter/simdhash.h index b939bc25fd93c2..6396e4fb325f2b 100644 --- a/src/coreclr/interpreter/simdhash.h +++ b/src/coreclr/interpreter/simdhash.h @@ -52,21 +52,21 @@ class dn_simdhash_ptr_ptr_holder }; // Asserts that no error occurred during a simdhash add, but does not mind if no new item was inserted -inline void assertNoError(dn_simdhash_add_result result) +inline void checkNoError(dn_simdhash_add_result result) { if (result == DN_SIMDHASH_OUT_OF_MEMORY) NOMEM(); else if (result < 0) - assert(!"Internal error in simdhash"); + NO_WAY("Internal error in simdhash"); } // Asserts that a new item was successfully inserted into the simdhash -inline void assertAddedNew(dn_simdhash_add_result result) +inline void checkAddedNew(dn_simdhash_add_result result) { if (result == DN_SIMDHASH_OUT_OF_MEMORY) NOMEM(); else if (result != DN_SIMDHASH_ADD_INSERTED) - assert(!"Failed to add new item into simdhash"); + NO_WAY("Failed to add new item into simdhash"); } #endif // _SIMDHASH_H_ diff --git a/src/coreclr/interpreter/stackmap.cpp b/src/coreclr/interpreter/stackmap.cpp index 5e2de5e35ea43f..525115f3ca6e79 100644 --- a/src/coreclr/interpreter/stackmap.cpp +++ b/src/coreclr/interpreter/stackmap.cpp @@ -11,7 +11,11 @@ void dn_simdhash_assert_fail (const char* file, int line, const char* condition) { +#if DEBUG assertAbort(condition, file, line); +#else + NO_WAY(condition); +#endif } thread_local dn_simdhash_ptr_ptr_t *t_sharedStackMapLookup = nullptr; @@ -27,7 +31,7 @@ InterpreterStackMap* GetInterpreterStackMap(ICorJitInfo* jitInfo, CORINFO_CLASS_ if (!dn_simdhash_ptr_ptr_try_get_value(t_sharedStackMapLookup, classHandle, (void **)&result)) { result = new InterpreterStackMap(jitInfo, classHandle); - assertAddedNew(dn_simdhash_ptr_ptr_try_add(t_sharedStackMapLookup, classHandle, result)); + checkAddedNew(dn_simdhash_ptr_ptr_try_add(t_sharedStackMapLookup, classHandle, result)); } return result; }