-
Notifications
You must be signed in to change notification settings - Fork 40
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
use generic to allow 32 and 64 uint keys #30
base: main
Are you sure you want to change the base?
Conversation
Feel free to squash commits. |
// lemire.me/blog/2016/06/27/a-fast-alternative-to-the-modulo-reduction/ | ||
func fastModN(x, n uint32) uint32 { | ||
return uint32((uint64(x) * uint64(n)) >> 32) | ||
func fastModN[S constraints.Unsigned](x, n S) S { | ||
return S((uint64(x) * uint64(n)) >> 32) |
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.
This is the part that will not work with uint64
: it will always return values in 32-bit range, so those buckets will be unused.
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 see. Would returning unsafe.Sizeof(S)
help here? Or is this a bit-op trick that can not be rescued?
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 don't think it can be rescued without having to do the usual modulo division. See the link above that explains how the trick works.
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.
This writeup shows a possible version to support uint64 ranges.
Perhaps there's a method here for assigning the correct fastmodN
for the map instantiation. It, however, wouldn't be inlined by the compiler.
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.
Thank you for the link! It's always great to learn something new.
Maybe it would make sense to just have a fork of this repo (swiss64?) with the 64-bit version? It should be quite easy to keep it up to date (I guess there shouldn't be many or frequent incoming changes), the API would be clean (swiss.Map
vs swiss64.Map
) and the code would be easier and faster to follow. WDYT?
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'm not sure. That seems like a dolthub call.
#27