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

Use of int for internal counter #31

Open
cpacia opened this issue Oct 20, 2023 · 2 comments
Open

Use of int for internal counter #31

cpacia opened this issue Oct 20, 2023 · 2 comments

Comments

@cpacia
Copy link

cpacia commented Oct 20, 2023

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.

@mroth
Copy link
Owner

mroth commented Oct 21, 2023

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.)

@cpacia
Copy link
Author

cpacia commented Oct 21, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants