Merge pull request #2535 from jasonKercher/fix2515

Fix #2515 - Implement backward shift on `map` on insert and reseed hashes on resize

core/runtime/dynamic_map_internal.odin

@@ -251,6 +251,26 @@ map_hash_is_valid :: #force_inline proc "contextless" (hash: Map_Hash) -> bool {
 	return (hash != 0) & (hash & TOMBSTONE_MASK == 0)
 }
 
+@(require_results)
+map_seed :: #force_inline proc "contextless" (m: Raw_Map) -> uintptr {
+	return map_seed_from_map_data(map_data(m))
+}
+
+// splitmix for uintptr
+@(require_results)
+map_seed_from_map_data :: #force_inline proc "contextless" (data: uintptr) -> uintptr {
+	when size_of(uintptr) == size_of(u64) {
+		mix := data + 0x9e3779b97f4a7c15
+		mix = (mix ~ (mix >> 30)) * 0xbf58476d1ce4e5b9
+		mix = (mix ~ (mix >> 27)) * 0x94d049bb133111eb
+		return mix ~ (mix >> 31)
+	} else {
+		mix := data + 0x9e3779b9
+		mix = (mix ~ (mix >> 16)) * 0x21f0aaad
+		mix = (mix ~ (mix >> 15)) * 0x735a2d97
+		return mix ~ (mix >> 15)
+	}
+}
 
 // Computes the desired position in the array. This is just index % capacity,
 // but a procedure as there's some math involved here to recover the capacity.
@@ -394,32 +414,71 @@ map_insert_hash_dynamic :: proc "odin" (#no_alias m: ^Raw_Map, #no_alias info: ^
 	tk := map_cell_index_dynamic(sk, info.ks, 1)
 	tv := map_cell_index_dynamic(sv, info.vs, 1)
 
-
 	for {
 		hp := &hs[pos]
 		element_hash := hp^
 
 		if map_hash_is_empty(element_hash) {
-			k_dst := map_cell_index_dynamic(ks, info.ks, pos)
-			v_dst := map_cell_index_dynamic(vs, info.vs, pos)
-			intrinsics.mem_copy_non_overlapping(rawptr(k_dst), rawptr(k), size_of_k)
-			intrinsics.mem_copy_non_overlapping(rawptr(v_dst), rawptr(v), size_of_v)
+			kp := map_cell_index_dynamic(ks, info.ks, pos)
+			vp := map_cell_index_dynamic(vs, info.vs, pos)
+			intrinsics.mem_copy_non_overlapping(rawptr(kp), rawptr(k), size_of_k)
+			intrinsics.mem_copy_non_overlapping(rawptr(vp), rawptr(v), size_of_v)
 			hp^ = h
 
-			return result if result != 0 else v_dst
+			return result if result != 0 else vp
 		}
 
-		if probe_distance := map_probe_distance(m^, element_hash, pos); distance > probe_distance {
-			if map_hash_is_deleted(element_hash) {
-				k_dst := map_cell_index_dynamic(ks, info.ks, pos)
-				v_dst := map_cell_index_dynamic(vs, info.vs, pos)
-				intrinsics.mem_copy_non_overlapping(rawptr(k_dst), rawptr(k), size_of_k)
-				intrinsics.mem_copy_non_overlapping(rawptr(v_dst), rawptr(v), size_of_v)
-				hp^ = h
-
-				return result if result != 0 else v_dst
+		if map_hash_is_deleted(element_hash) {
+			next_pos := (pos + 1) & mask
+
+			// backward shift
+			for !map_hash_is_empty(hs[next_pos]) {
+				probe_distance := map_probe_distance(m^, hs[next_pos], next_pos)
+				if probe_distance == 0 {
+					break
+				}
+				probe_distance -= 1
+
+				kp := map_cell_index_dynamic(ks, info.ks, pos)
+				vp := map_cell_index_dynamic(vs, info.vs, pos)
+				kn := map_cell_index_dynamic(ks, info.ks, next_pos)
+				vn := map_cell_index_dynamic(vs, info.vs, next_pos)
+
+				if distance > probe_distance {
+					if result == 0 {
+						result = vp
+					}
+					// move stored into pos; store next
+					intrinsics.mem_copy_non_overlapping(rawptr(kp), rawptr(k), size_of_k)
+					intrinsics.mem_copy_non_overlapping(rawptr(vp), rawptr(v), size_of_v)
+					hs[pos] = h
+
+					intrinsics.mem_copy_non_overlapping(rawptr(k), rawptr(kn), size_of_k)
+					intrinsics.mem_copy_non_overlapping(rawptr(v), rawptr(vn), size_of_v)
+					h = hs[next_pos]
+				} else {
+					// move next back 1
+					intrinsics.mem_copy_non_overlapping(rawptr(kp), rawptr(kn), size_of_k)
+					intrinsics.mem_copy_non_overlapping(rawptr(vp), rawptr(vn), size_of_v)
+					hs[pos] = hs[next_pos]
+					distance = probe_distance
+				}
+				hs[next_pos] = 0
+				pos = (pos + 1) & mask
+				next_pos = (next_pos + 1) & mask
+				distance += 1
 			}
 
+			kp := map_cell_index_dynamic(ks, info.ks, pos)
+			vp := map_cell_index_dynamic(vs, info.vs, pos)
+			intrinsics.mem_copy_non_overlapping(rawptr(kp), rawptr(k), size_of_k)
+			intrinsics.mem_copy_non_overlapping(rawptr(vp), rawptr(v), size_of_v)
+			hs[pos] = h
+
+			return result if result != 0 else vp
+		}
+
+		if probe_distance := map_probe_distance(m^, element_hash, pos); distance > probe_distance {
 			if result == 0 {
 				result = map_cell_index_dynamic(vs, info.vs, pos)
 			}
@@ -503,6 +562,7 @@ map_reserve_dynamic :: proc "odin" (#no_alias m: ^Raw_Map, #no_alias info: ^Map_
 		}
 		k := map_cell_index_dynamic(ks, info.ks, i)
 		v := map_cell_index_dynamic(vs, info.vs, i)
+		hash = info.key_hasher(rawptr(k), map_seed(resized))
 		_ = map_insert_hash_dynamic(&resized, info, hash, k, v)
 		// Only need to do this comparison on each actually added pair, so do not
 		// fold it into the for loop comparator as a micro-optimization.
@@ -550,6 +610,7 @@ map_shrink_dynamic :: proc "odin" (#no_alias m: ^Raw_Map, #no_alias info: ^Map_I
 
 		k := map_cell_index_dynamic(ks, info.ks, i)
 		v := map_cell_index_dynamic(vs, info.vs, i)
+		hash = info.key_hasher(rawptr(k), map_seed(shrunk))
 		_ = map_insert_hash_dynamic(&shrunk, info, hash, k, v)
 		// Only need to do this comparison on each actually added pair, so do not
 		// fold it into the for loop comparator as a micro-optimization.
@@ -581,7 +642,7 @@ map_lookup_dynamic :: proc "contextless" (m: Raw_Map, #no_alias info: ^Map_Info,
 	if map_len(m) == 0 {
 		return 0, false
 	}
-	h := info.key_hasher(rawptr(k), 0)
+	h := info.key_hasher(rawptr(k), map_seed(m))
 	p := map_desired_position(m, h)
 	d := uintptr(0)
 	c := (uintptr(1) << map_log2_cap(m)) - 1
@@ -604,7 +665,7 @@ map_exists_dynamic :: proc "contextless" (m: Raw_Map, #no_alias info: ^Map_Info,
 	if map_len(m) == 0 {
 		return false
 	}
-	h := info.key_hasher(rawptr(k), 0)
+	h := info.key_hasher(rawptr(k), map_seed(m))
 	p := map_desired_position(m, h)
 	d := uintptr(0)
 	c := (uintptr(1) << map_log2_cap(m)) - 1
@@ -637,7 +698,6 @@ map_erase_dynamic :: #force_inline proc "contextless" (#no_alias m: ^Raw_Map, #n
 
 	{ // coalesce tombstones
 		// HACK NOTE(bill): This is an ugly bodge but it is coalescing the tombstone slots
-		// TODO(bill): we should do backward shift deletion and not rely on tombstone slots
 		mask := (uintptr(1)<<map_log2_cap(m^)) - 1
 		curr_index := uintptr(index)
 
@@ -711,7 +771,7 @@ map_get :: proc "contextless" (m: $T/map[$K]$V, key: K) -> (stored_key: K, store
 	info := intrinsics.type_map_info(T)
 	key := key
 
-	h := info.key_hasher(&key, 0)
+	h := info.key_hasher(&key, map_seed(m))
 	pos := map_desired_position(rm, h)
 	distance := uintptr(0)
 	mask := (uintptr(1) << map_log2_cap(rm)) - 1
@@ -762,15 +822,15 @@ __dynamic_map_get :: proc "contextless" (#no_alias m: ^Raw_Map, #no_alias info:
 }
 
 // IMPORTANT: USED WITHIN THE COMPILER
-__dynamic_map_check_grow :: proc "odin" (#no_alias m: ^Raw_Map, #no_alias info: ^Map_Info, loc := #caller_location) -> Allocator_Error {
+__dynamic_map_check_grow :: proc "odin" (#no_alias m: ^Raw_Map, #no_alias info: ^Map_Info, loc := #caller_location) -> (err: Allocator_Error, has_grown: bool) {
 	if m.len >= map_resize_threshold(m^) {
-		return map_grow_dynamic(m, info, loc)
+		return map_grow_dynamic(m, info, loc), true
 	}
-	return nil
+	return nil, false
 }
 
 __dynamic_map_set_without_hash :: proc "odin" (#no_alias m: ^Raw_Map, #no_alias info: ^Map_Info, key, value: rawptr, loc := #caller_location) -> rawptr {
-	return __dynamic_map_set(m, info, info.key_hasher(key, 0), key, value, loc)
+	return __dynamic_map_set(m, info, info.key_hasher(key, map_seed(m^)), key, value, loc)
 }
 
 
@@ -781,9 +841,14 @@ __dynamic_map_set :: proc "odin" (#no_alias m: ^Raw_Map, #no_alias info: ^Map_In
 		return found
 	}
 
-	if __dynamic_map_check_grow(m, info, loc) != nil {
+	hash := hash
+	err, has_grown := __dynamic_map_check_grow(m, info, loc)
+	if err != nil {
 		return nil
 	}
+	if has_grown {
+		hash = info.key_hasher(key, map_seed(m^))
+	}
 
 	result := map_insert_hash_dynamic(m, info, hash, uintptr(key), uintptr(value))
 	m.len += 1

src/llvm_backend.cpp

@@ -720,6 +720,7 @@ gb_internal lbValue lb_map_set_proc_for_type(lbModule *m, Type *type) {
 	lbBlock *check_grow_block = lb_create_block(p, "check-grow");
 	lbBlock *grow_fail_block  = lb_create_block(p, "grow-fail");
 	lbBlock *insert_block     = lb_create_block(p, "insert");
+	lbBlock *rehash_block     = lb_create_block(p, "rehash");
 
 	lb_emit_if(p, lb_emit_comp_against_nil(p, Token_NotEq, found_ptr), found_block, check_grow_block);
 	lb_start_block(p, found_block);
@@ -737,12 +738,19 @@ gb_internal lbValue lb_map_set_proc_for_type(lbModule *m, Type *type) {
 		args[0] = lb_emit_conv(p, map_ptr, t_rawptr);
 		args[1] = map_info;
 		args[2] = lb_emit_load(p, location_ptr);
-		lbValue grow_err = lb_emit_runtime_call(p, "__dynamic_map_check_grow", args);
+		lbValue grow_err_and_has_grown = lb_emit_runtime_call(p, "__dynamic_map_check_grow", args);
+		lbValue grow_err = lb_emit_struct_ev(p, grow_err_and_has_grown, 0);
+		lbValue has_grown = lb_emit_struct_ev(p, grow_err_and_has_grown, 1);
 
 		lb_emit_if(p, lb_emit_comp_against_nil(p, Token_NotEq, grow_err), grow_fail_block, insert_block);
 
 		lb_start_block(p, grow_fail_block);
 		LLVMBuildRet(p->builder, LLVMConstNull(lb_type(m, t_rawptr)));
+
+		lb_emit_if(p, has_grown, grow_fail_block, rehash_block);
+		lb_start_block(p, rehash_block);
+		lbValue key = lb_emit_load(p, key_ptr);
+		hash = lb_gen_map_key_hash(p, map_ptr, key, nullptr);
 	}
 
 	lb_start_block(p, insert_block);
@@ -916,7 +924,7 @@ gb_internal lbValue lb_const_hash(lbModule *m, lbValue key, Type *key_type) {
 	return hashed_key;
 }
 
-gb_internal lbValue lb_gen_map_key_hash(lbProcedure *p, lbValue key, Type *key_type, lbValue *key_ptr_) {
+gb_internal lbValue lb_gen_map_key_hash(lbProcedure *p, lbValue const &map_ptr, lbValue key, lbValue *key_ptr_) {
 	TEMPORARY_ALLOCATOR_GUARD();
 
 	lbValue key_ptr = lb_address_from_load_or_generate_local(p, key);
@@ -924,13 +932,22 @@ gb_internal lbValue lb_gen_map_key_hash(lbProcedure *p, lbValue key, Type *key_t
 
 	if (key_ptr_) *key_ptr_ = key_ptr;
 
+	Type* key_type = base_type(type_deref(map_ptr.type))->Map.key;
+
 	lbValue hashed_key = lb_const_hash(p->module, key, key_type);
 	if (hashed_key.value == nullptr) {
 		lbValue hasher = lb_hasher_proc_for_type(p->module, key_type);
 
+		lbValue seed = {};
+		{
+			auto args = array_make<lbValue>(temporary_allocator(), 1);
+			args[0] = lb_map_data_uintptr(p, lb_emit_load(p, map_ptr));
+			seed = lb_emit_runtime_call(p, "map_seed_from_map_data", args);
+		}
+
 		auto args = array_make<lbValue>(temporary_allocator(), 2);
 		args[0] = key_ptr;
-		args[1] = lb_const_int(p->module, t_uintptr, 0);
+		args[1] = seed;
 		hashed_key = lb_emit_call(p, hasher, args);
 	}
 
@@ -945,7 +962,7 @@ gb_internal lbValue lb_internal_dynamic_map_get_ptr(lbProcedure *p, lbValue cons
 
 	lbValue ptr = {};
 	lbValue key_ptr = {};
-	lbValue hash = lb_gen_map_key_hash(p, key, map_type->Map.key, &key_ptr);
+	lbValue hash = lb_gen_map_key_hash(p, map_ptr, key, &key_ptr);
 
 	if (build_context.dynamic_map_calls) {
 		auto args = array_make<lbValue>(temporary_allocator(), 4);
@@ -976,7 +993,7 @@ gb_internal void lb_internal_dynamic_map_set(lbProcedure *p, lbValue const &map_
 	GB_ASSERT(map_type->kind == Type_Map);
 
 	lbValue key_ptr = {};
-	lbValue hash = lb_gen_map_key_hash(p, map_key, map_type->Map.key, &key_ptr);
+	lbValue hash = lb_gen_map_key_hash(p, map_ptr, map_key, &key_ptr);
 
 	lbValue v = lb_emit_conv(p, map_value, map_type->Map.value);
 	lbValue value_ptr = lb_address_from_load_or_generate_local(p, v);

src/llvm_backend.hpp

@@ -477,7 +477,7 @@ gb_internal String lb_get_const_string(lbModule *m, lbValue value);
 
 gb_internal lbValue lb_generate_local_array(lbProcedure *p, Type *elem_type, i64 count, bool zero_init=true);
 gb_internal lbValue lb_generate_global_array(lbModule *m, Type *elem_type, i64 count, String prefix, i64 id);
-gb_internal lbValue lb_gen_map_key_hash(lbProcedure *p, lbValue key, Type *key_type, lbValue *key_ptr_);
+gb_internal lbValue lb_gen_map_key_hash(lbProcedure *p, lbValue const &map_ptr, lbValue key, lbValue *key_ptr_);
 gb_internal lbValue lb_gen_map_cell_info_ptr(lbModule *m, Type *type);
 gb_internal lbValue lb_gen_map_info_ptr(lbModule *m, Type *map_type);