Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime: map_cell_index_static produced wrong results when the number of elements per cell was a power of 2 #4651

Merged
merged 4 commits into from
Jan 5, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions base/runtime/dynamic_map_internal.odin
Original file line number Diff line number Diff line change
Expand Up @@ -158,21 +158,21 @@ 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.
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 {
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
Expand Down
99 changes: 98 additions & 1 deletion tests/core/runtime/test_core_runtime.odin
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,101 @@ 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)
}
}

@(test)
test_map_get :: proc(t: ^testing.T) {
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)
}
}

// small keys & values
{
m := map[int]int{
1 = 10,
2 = 20,
3 = 30,
}
defer delete(m)
check(t, m)
}

// 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)
}

// 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; 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{
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)
}
}
Loading