-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
hash_map.zig: Pass self
by value and less pointer-int conversion
#19989
hash_map.zig: Pass self
by value and less pointer-int conversion
#19989
Conversation
- Used `Self` instead of `*const Self` where appropriate (orignally proposed in ziglang#19770) - Replaced `@intFromPtr` and `@ptrFromInt` with `@ptrCast`, `@alignCast`, and pointer arithmetic where appropriate With this, the only remaining instance on pointer-int conversion in hash_map.zig is in `HashMapUnmanaged.removeByPtr`, which easily be able to be eliminated once pointer subtraction is supported.
Would be great to run some benchmarks before/after, to make sure this doesn't regress performance. My understanding is that this should improve performance in the ideal Zig, but also that the current version of Zig isn't quiet ideal yet. |
@matklad see #19770 (comment):
and
|
Yes, I am aware of that context. The expected value of benchmarking here is still quiet high:
Not saying that we shouldn't land this without a benchmark, or that we should block this on compiler improvements should the results be disappointing! Just that with a benchmark this change would be even more valuable than it already is: if it is a performance regression, chasing it after the fact would be quite hard, but also important, because this is hash-map specifically, and not some random cold code! |
Alright, I've finally gotten around to doing some benchmarks. If you want to run the benchmark on your own machine, all of the code is at this repo, and you can run the benchmark with To be comprehensive, here is my benchmark results for ReleaseFast mode:
And here is my benchmark results for Debug mode:
|
HashMapUnmanged
andHashMap
(orignally proposed in fix(HashMap): changecapacity()
to*const Self
#19770)@intFromPtr
and@ptrFromInt
with@ptrCast
and@alignCast
, and pointer arithmetic where appropriateWith this, the only remaining instance of pointer-int conversion in hash_map.zig is in
HashMapUnmanaged.removeByPtr
, which easily be able to be eliminated once pointer subtraction is supported.