Use of int for internal counter
Opened this issue · 2 comments
Wouldn't it make more sense to use a uint64 for the total and max fields inside the chooser?
As it stands you allow the user to use a uint64 for the weight but the max weight total cannot exceed a maxInt. And that maxInt is different for users on 32 and 64 bit systems creating uncertainty in how the software will run on users computers.
I've had a bug report of my software being unable to run on 32 bit systems because I need the total weight to exceed a maxInt32 (and ideally maxInt64 for that matter).
It seems like if you're going to allow uint64s as weights then internally the counter should be able to handle that maximum value.
I believe this would be possible now.
However, it would require a performance optimized version of sort.SearchInts that could handle uint64s (as per my benchmarks in mroth/xsort#1, the new slices.BinarySearch should be promising here once some nits are addressed in go1.22)
It would also require an implementation of rand.Uint64n, not currently in the standard library but should also be in go1.22 in the new math/rand/v2 module (also exists in the golang.org/exp/rand repo but I'd rather not pull in an exp dependency).
So the options would be to roll my own versions of the above functions (which makes me a little nervous, but is very feasible with enough testing -- I've definitely hit weird unexpected edge case bugs here in the past due to obscure int/uint behaviors so testing is required), or wait ~9mo and roll a v3 version of this library that requires >=go1.22. FWIW I already planned to do so upon release of go1.23 (I target at least stable and 'oldstable') in order to use the new math/rand/v2 which I'm quite excited about, so its a question of whether it's worth doing the work of a custom implementation for v2 of this library for the next N months.
Thoughts welcome. (I'm also curious what the use case is where you frequently need to be able to exceed maxInt64 with weights -- as I'm guessing a big.Int implementation is likely to have substantial performance hits.)
Sounds good.
The use case i have is cryptocurrency related. The weight is a number of coins. The total coins on the network wont exceed MaxInt64 for 100 years so Int64 is fine, but in theory the number could go higher.