From e3de02eaa8b69ae615c3df1177f44f326ca5ac44 Mon Sep 17 00:00:00 2001 From: fleandro <3987005+flga@users.noreply.github.com> Date: Fri, 3 Jan 2025 15:33:34 +0000 Subject: [PATCH 1/4] runtime: map_cell_index_static produced wrong results when the number of elements per cell was a power of 2 --- base/runtime/dynamic_map_internal.odin | 10 ++++----- tests/core/runtime/test_core_runtime.odin | 26 ++++++++++++++++++++++- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/base/runtime/dynamic_map_internal.odin b/base/runtime/dynamic_map_internal.odin index 3dded7716..281d4b88e 100644 --- a/base/runtime/dynamic_map_internal.odin +++ b/base/runtime/dynamic_map_internal.odin @@ -161,11 +161,11 @@ map_cell_index_static :: #force_inline proc "contextless" (cells: [^]Map_Cell($T // Compute the integer log 2 of N, this is the shift amount to index the // correct cell. Odin's intrinsics.count_leading_zeros does not produce a // constant, hence this approach. We only need to check up to N = 64. - SHIFT :: 1 when N < 2 else - 2 when N < 4 else - 3 when N < 8 else - 4 when N < 16 else - 5 when N < 32 else 6 + SHIFT :: 1 when N == 2 else + 2 when N == 4 else + 3 when N == 8 else + 4 when N == 16 else + 5 when N == 32 else 6 #assert(SHIFT <= MAP_CACHE_LINE_LOG2) // Unique case, no need to index data here since only one element. when N == 1 { diff --git a/tests/core/runtime/test_core_runtime.odin b/tests/core/runtime/test_core_runtime.odin index 84fd044cf..bd641ab3a 100644 --- a/tests/core/runtime/test_core_runtime.odin +++ b/tests/core/runtime/test_core_runtime.odin @@ -63,4 +63,28 @@ test_init_cap_map_dynarray :: proc(t: ^testing.T) { defer delete(d2) testing.expect(t, cap(d2) == 0) testing.expect(t, d2.allocator.procedure == ally.procedure) -} \ No newline at end of file +} + +@(test) +test_map_get :: proc(t: ^testing.T) { + m := map[int][3]int{ + 1 = {10, 100, 1000}, + 2 = {20, 200, 2000}, + 3 = {30, 300, 3000}, + } + + k1, v1, ok1 := runtime.map_get(m, 1) + testing.expect_value(t, k1, 1) + testing.expect_value(t, v1, [3]int{10, 100, 1000}) + testing.expect_value(t, ok1, true) + + k2, v2, ok2 := runtime.map_get(m, 2) + testing.expect_value(t, k2, 2) + testing.expect_value(t, v2, [3]int{20, 200, 2000}) + testing.expect_value(t, ok2, true) + + k3, v3, ok3 := runtime.map_get(m, 3) + testing.expect_value(t, k3, 3) + testing.expect_value(t, v3, [3]int{30, 300, 3000}) + testing.expect_value(t, ok3, true) +} From 555bca2cb4adf964c97b333646c432cd8fa9392a Mon Sep 17 00:00:00 2001 From: fleandro <3987005+flga@users.noreply.github.com> Date: Fri, 3 Jan 2025 15:46:02 +0000 Subject: [PATCH 2/4] fix test leaks --- tests/core/runtime/test_core_runtime.odin | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/core/runtime/test_core_runtime.odin b/tests/core/runtime/test_core_runtime.odin index bd641ab3a..ccadcec27 100644 --- a/tests/core/runtime/test_core_runtime.odin +++ b/tests/core/runtime/test_core_runtime.odin @@ -72,6 +72,7 @@ test_map_get :: proc(t: ^testing.T) { 2 = {20, 200, 2000}, 3 = {30, 300, 3000}, } + defer delete(m) k1, v1, ok1 := runtime.map_get(m, 1) testing.expect_value(t, k1, 1) From c93e096d8f39cc97739e15a32f48ecfe83829593 Mon Sep 17 00:00:00 2001 From: fleandro <3987005+flga@users.noreply.github.com> Date: Fri, 3 Jan 2025 16:50:05 +0000 Subject: [PATCH 3/4] fix N=1 and cleanup tests --- base/runtime/dynamic_map_internal.odin | 12 ++-- tests/core/runtime/test_core_runtime.odin | 75 ++++++++++++++++++----- 2 files changed, 64 insertions(+), 23 deletions(-) diff --git a/base/runtime/dynamic_map_internal.odin b/base/runtime/dynamic_map_internal.odin index 281d4b88e..4e22aa25c 100644 --- a/base/runtime/dynamic_map_internal.odin +++ b/base/runtime/dynamic_map_internal.odin @@ -158,6 +158,11 @@ map_cell_index_static :: #force_inline proc "contextless" (cells: [^]Map_Cell($T } else when (N & (N - 1)) == 0 && N <= 8*size_of(uintptr) { // Likely case, N is a power of two because T is a power of two. + // Unique case, no need to index data here since only one element. + when N == 1 { + return &cells[index].data[0] + } + // Compute the integer log 2 of N, this is the shift amount to index the // correct cell. Odin's intrinsics.count_leading_zeros does not produce a // constant, hence this approach. We only need to check up to N = 64. @@ -167,12 +172,7 @@ map_cell_index_static :: #force_inline proc "contextless" (cells: [^]Map_Cell($T 4 when N == 16 else 5 when N == 32 else 6 #assert(SHIFT <= MAP_CACHE_LINE_LOG2) - // Unique case, no need to index data here since only one element. - when N == 1 { - return &cells[index >> SHIFT].data[0] - } else { - return &cells[index >> SHIFT].data[index & (N - 1)] - } + return &cells[index >> SHIFT].data[index & (N - 1)] } else { // Least likely (and worst case), we pay for a division operation but we // assume the compiler does not actually generate a division. N will be in the diff --git a/tests/core/runtime/test_core_runtime.odin b/tests/core/runtime/test_core_runtime.odin index ccadcec27..d2ced2504 100644 --- a/tests/core/runtime/test_core_runtime.odin +++ b/tests/core/runtime/test_core_runtime.odin @@ -67,25 +67,66 @@ test_init_cap_map_dynarray :: proc(t: ^testing.T) { @(test) test_map_get :: proc(t: ^testing.T) { - m := map[int][3]int{ - 1 = {10, 100, 1000}, - 2 = {20, 200, 2000}, - 3 = {30, 300, 3000}, + check :: proc(t: ^testing.T, m: map[$K]$V, loc := #caller_location) { + for k, v in m { + got_key, got_val, ok := runtime.map_get(m, k) + testing.expect_value(t, got_key, k, loc = loc) + testing.expect_value(t, got_val, v, loc = loc) + testing.expect(t, ok, loc = loc) + } } - defer delete(m) - k1, v1, ok1 := runtime.map_get(m, 1) - testing.expect_value(t, k1, 1) - testing.expect_value(t, v1, [3]int{10, 100, 1000}) - testing.expect_value(t, ok1, true) + // small keys & values + { + m := map[int]int{ + 1 = 10, + 2 = 20, + 3 = 30, + } + defer delete(m) + check(t, m) + } - k2, v2, ok2 := runtime.map_get(m, 2) - testing.expect_value(t, k2, 2) - testing.expect_value(t, v2, [3]int{20, 200, 2000}) - testing.expect_value(t, ok2, true) + // small keys; 2 values per cell + { + m := map[int][3]int{ + 1 = [3]int{10, 100, 1000}, + 2 = [3]int{20, 200, 2000}, + 3 = [3]int{30, 300, 3000}, + } + defer delete(m) + check(t, m) + } - k3, v3, ok3 := runtime.map_get(m, 3) - testing.expect_value(t, k3, 3) - testing.expect_value(t, v3, [3]int{30, 300, 3000}) - testing.expect_value(t, ok3, true) + // 2 keys per cell; small values + { + m := map[[3]int]int{ + [3]int{10, 100, 1000} = 1, + [3]int{20, 200, 2000} = 2, + [3]int{30, 300, 3000} = 3, + } + defer delete(m) + check(t, m) + } + + // small keys; value bigger than a chacheline + { + m := map[int][9]int{ + 1 = [9]int{10, 100, 1000, 10000, 100000, 1000000, 10000000, 100000000, 1000000000}, + 2 = [9]int{20, 200, 2000, 20000, 200000, 2000000, 20000000, 200000000, 2000000000}, + 3 = [9]int{30, 300, 3000, 30000, 300000, 3000000, 30000000, 300000000, 3000000000}, + } + defer delete(m) + check(t, m) + } + // keys bigger than a chacheline; small values + { + m := map[[9]int]int{ + [9]int{10, 100, 1000, 10000, 100000, 1000000, 10000000, 100000000, 1000000000} = 1, + [9]int{20, 200, 2000, 20000, 200000, 2000000, 20000000, 200000000, 2000000000} = 2, + [9]int{30, 300, 3000, 30000, 300000, 3000000, 30000000, 300000000, 3000000000} = 3, + } + defer delete(m) + check(t, m) + } } From 1550eced046c61fed1121fa6245af6d564ba3149 Mon Sep 17 00:00:00 2001 From: fleandro <3987005+flga@users.noreply.github.com> Date: Fri, 3 Jan 2025 16:58:03 +0000 Subject: [PATCH 4/4] also add a test for non power of 2 N for good measure --- tests/core/runtime/test_core_runtime.odin | 31 +++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/core/runtime/test_core_runtime.odin b/tests/core/runtime/test_core_runtime.odin index d2ced2504..c1a3ed718 100644 --- a/tests/core/runtime/test_core_runtime.odin +++ b/tests/core/runtime/test_core_runtime.odin @@ -109,6 +109,37 @@ test_map_get :: proc(t: ^testing.T) { check(t, m) } + + // small keys; 3 values per cell + { + val :: struct #packed { + a, b: int, + c: i32, + } + m := map[int]val{ + 1 = val{10, 100, 1000}, + 2 = val{20, 200, 2000}, + 3 = val{30, 300, 3000}, + } + defer delete(m) + check(t, m) + } + + // 3 keys per cell; small values + { + key :: struct #packed { + a, b: int, + c: i32, + } + m := map[key]int{ + key{10, 100, 1000} = 1, + key{20, 200, 2000} = 2, + key{30, 300, 3000} = 3, + } + defer delete(m) + check(t, m) + } + // small keys; value bigger than a chacheline { m := map[int][9]int{