-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
std.ArrayHashMap: base linear_scan_max on cache line size #22861
Conversation
61e076b
to
8c38d5b
Compare
lib/std/array_hash_map.zig
Outdated
const linear_scan_max = @as(comptime_int, @max(1, @as(comptime_int, @min( | ||
std.atomic.cache_line / @as(comptime_int, @max(1, @sizeOf(Hash))), | ||
std.atomic.cache_line / @as(comptime_int, @max(1, @sizeOf(K))), | ||
std.atomic.cache_line / @as(comptime_int, @max(1, @sizeOf(V))), | ||
)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not? It seems to produce identical result
const linear_scan_max = @as(comptime_int, @max(1, @as(comptime_int, @min( | |
std.atomic.cache_line / @as(comptime_int, @max(1, @sizeOf(Hash))), | |
std.atomic.cache_line / @as(comptime_int, @max(1, @sizeOf(K))), | |
std.atomic.cache_line / @as(comptime_int, @max(1, @sizeOf(V))), | |
)))); | |
const linear_scan_max: comptime_int = @max(1, @min( | |
std.atomic.cache_line / @max(1, @sizeOf(Hash)), | |
std.atomic.cache_line / @max(1, @sizeOf(K)), | |
std.atomic.cache_line / @max(1, @sizeOf(V)), | |
)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compile error because @max
and @min
(incorrectly) return a sized integer even when the value is comptime-known
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comptime_int
is an awkward type which is fundamentally at odds with the range refinement we want @min
/@max
to do -- our saviour will hopefully be #3806
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree; comptime_int is basically a ranged integer with the min and max set to the same value. Strongly believe that @max
and @min
should return comptime_int when the value is comptime known given that #3806 is not implemented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would lead to a major behavioral inconsistency between runtime and comptime.
EDIT: oh, to be fair, I guess @min
/@max
already has this issue, so disregard that point. TBH, its range refinement is necessarily kind of weird in the absence of #3806.
8c38d5b
to
68241c6
Compare
Pushed an update to only consider hash and key sizes since that's what is iterated over when doing linear scans. Performance seems to be insignificant difference compared to status quo. build zig with itself:
wasm linker hello world (wasm linker uses a lot of array hash map):
I'm going to keep the change because it feels good 🌈 |
I have a hunch that this will be a better default. Will post perf data points shortly.