-
-
Notifications
You must be signed in to change notification settings - Fork 48
v3 release #36
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
base: main
Are you sure you want to change the base?
v3 release #36
Conversation
|
I'm very excited to improve. Can I help you, @mroth? |
|
As a user of v2, I think the API looks good here. As a bonus, it would be nice to rename your c, _ := weightedrand.NewChooser(...)
c.SetRand(r)it's more convenient to do it all in a single call. If you're on the fence about including the Other than that, it all seems reasonable to me, and I'm excited for when it's released! |
Thank you for the helpful and thoughtful feedback @robinjoseph08 ! Sorry for the delay in response.
Yeah, I can see that. My worry with the ergonomics is that since // generic type constraints omitted in examples here to aid in clarity of readability:
func NewChooser(choices ...Choice) (*Chooser, error)
func NewChooserWithRand(choices []Choice, r *rand.Rand) (*Chooser, error)But this presents other ergonomics issues due to the inconsistency (and I generally do think its better to avoid requiring slice construction in the call site, so end-users to not have to deal with declaring A third alternative could be to offer a chainable
Good points all. I was hoping to reintroduce some way to do this in v3 (due to requests in #29), but this is currently the design decision that is blocking getting to a release... |
This a first step in refactoring towards a v3 release. A major semantic
version release is required in order to let us fully remove the
deprecated PickSource from our API.
Switches to using the new math/rand/v2 module. Paving the way for the
future.
Removes PickSource method: As planned, this will remove the previously
deprecated PickSource method that uses v1 of math/rand sources. Custom
randomness source should be reintroduced in a future version using a
different methodology (e.g. setting on the Chooser instead of with each
function call).
Switches the Chooser backing from being a system int to a uint64. This
should result in defined behavior across 32 and 64 bit systems (with
the potential for some performance regressions on 32 bit systems, which
I consider an acceptable tradeoff).
Internal implementation: removes the custom searchInts binary sort that
was present for performance (ala github.com/mroth/xsort)in favor of
slices.BinarySearch which is available as of go1.21 and hits acceptable
performance as of go1.22.
goos: darwin
goarch: arm64
pkg: github.com/mroth/weightedrand/v2
│ v2-main.txt │ v3-dev1.txt │
│ sec/op │ sec/op vs base │
NewChooser/size=1e1-8 132.7n ± 0% 132.9n ± 0% ~ (p=0.314 n=6)
NewChooser/size=1e2-8 476.1n ± 1% 472.8n ± 0% -0.70% (p=0.002 n=6)
NewChooser/size=1e3-8 3.406µ ± 0% 3.412µ ± 0% ~ (p=0.379 n=6)
NewChooser/size=1e4-8 31.19µ ± 1% 31.03µ ± 0% -0.51% (p=0.002 n=6)
NewChooser/size=1e5-8 296.6µ ± 0% 295.9µ ± 0% ~ (p=0.394 n=6)
NewChooser/size=1e6-8 2.843m ± 1% 2.843m ± 1% ~ (p=0.485 n=6)
NewChooser/size=1e7-8 35.83m ± 1% 35.92m ± 1% ~ (p=0.485 n=6)
Pick/size=1e1-8 22.49n ± 8% 20.28n ± 9% -9.80% (p=0.015 n=6)
Pick/size=1e2-8 35.26n ± 2% 32.82n ± 2% -6.92% (p=0.002 n=6)
Pick/size=1e3-8 48.41n ± 1% 45.38n ± 1% -6.26% (p=0.002 n=6)
Pick/size=1e4-8 63.30n ± 1% 60.23n ± 2% -4.85% (p=0.002 n=6)
Pick/size=1e5-8 85.92n ± 1% 82.53n ± 1% -3.95% (p=0.002 n=6)
Pick/size=1e6-8 111.5n ± 1% 107.3n ± 4% -3.72% (p=0.013 n=6)
Pick/size=1e7-8 240.7n ± 2% 233.2n ± 1% -3.10% (p=0.002 n=6)
PickParallel/size=1e1-8 2.982n ± 6% 2.760n ± 5% -7.43% (p=0.009 n=6)
PickParallel/size=1e2-8 4.679n ± 1% 4.360n ± 2% -6.81% (p=0.002 n=6)
PickParallel/size=1e3-8 6.422n ± 2% 6.059n ± 1% -5.66% (p=0.002 n=6)
PickParallel/size=1e4-8 8.463n ± 0% 8.114n ± 58% ~ (p=0.058 n=6)
PickParallel/size=1e5-8 11.55n ± 3% 11.06n ± 0% -4.24% (p=0.002 n=6)
PickParallel/size=1e6-8 14.98n ± 0% 14.40n ± 0% -3.87% (p=0.002 n=6)
PickParallel/size=1e7-8 34.70n ± 0% 33.71n ± 0% -2.82% (p=0.002 n=6)
PickSourceParallel/size=1e1-8 2.752n ± 10%
PickSourceParallel/size=1e2-8 4.369n ± 2%
PickSourceParallel/size=1e3-8 5.989n ± 1%
PickSourceParallel/size=1e4-8 7.991n ± 2%
PickSourceParallel/size=1e5-8 11.28n ± 0%
PickSourceParallel/size=1e6-8 14.59n ± 0%
PickSourceParallel/size=1e7-8 33.86n ± 0%
geomean 120.0n 279.7n -3.59% ¹
¹ benchmark set differs from baseline; geomeans may not be comparable
Since we're already requiring the slices package in stdlib with this
refactor, we can utilize this newer function which should be slightly
more efficient (and has nicer ergonomics imo).
Performs roughly ~11% faster during NewChooser initialization.
goos: darwin
goarch: arm64
pkg: github.com/mroth/weightedrand/v2
│ v3-dev1.txt │ v3-dev2.txt │
│ sec/op │ sec/op vs base │
NewChooser/size=1e1-8 132.90n ± 0% 70.73n ± 1% -46.78% (p=0.002 n=6)
NewChooser/size=1e2-8 472.8n ± 0% 444.1n ± 2% -6.05% (p=0.002 n=6)
NewChooser/size=1e3-8 3.412µ ± 0% 3.333µ ± 0% -2.30% (p=0.002 n=6)
NewChooser/size=1e4-8 31.03µ ± 0% 30.30µ ± 0% -2.33% (p=0.002 n=6)
NewChooser/size=1e5-8 295.9µ ± 0% 291.9µ ± 1% -1.36% (p=0.002 n=6)
NewChooser/size=1e6-8 2.843m ± 1% 2.775m ± 1% -2.37% (p=0.002 n=6)
NewChooser/size=1e7-8 35.92m ± 1% 32.99m ± 2% -8.16% (p=0.002 n=6)
geomean 279.7n 36.41µ -11.60% ¹
¹ benchmark set differs from baseline; geomeans may not be comparable
The internal mechanics of this are a bit inelegant, since unfortunately the global randomness source is not exported, necessitating these nil check methods instead. The API here needs some user feedback. I believe the majority case will want to set this once and not on a per-call basis (cf. the deprecated PickSource method in the previous version), but that needs be validated.
|
After some more thought on the API for custom randomness sources, I've landed on a simpler approach than the Instead of a setter that adds mutable state to the Chooser, I'm going with a dual-method pattern similar to what c.Pick() // uses global rand (the common case)
c.PickWith(rs) // uses provided *rand.Rand sourceThis keeps Chooser stateless/immutable after construction, makes the randomness source explicit at the call site, and avoids any potential for surprising behavior in concurrent code. For the v2 to v3 migration, users of the deprecated PickSource would just rename to PickWith (and update their rand source construction for math/rand/v2). I did some research into how other Go libraries handle this pattern (google/uuid, gofrs/uuid, oklog/ulid, gonum/sampleuv) and this felt like the cleanest fit. Planning to move forward with this approach soon unless anyone has concerns or alternative suggestions. |
Replace mutable SetRand() method with immutable PickWith(r *rand.Rand), following the dual-method pattern used by oklog/ulid and similar libraries. Addresses #29.
v3 is a breaking change release that migrates to
math/rand/v2and requires Go 1.22 or greater.Changes from v2
Breaking Changes
github.com/mroth/weightedrand/v3math/rand/v2)PickSource: Replaced byPickWith(same functionality usingmath/rand/v2source)Non-Breaking Changes
uint64: Allows larger weight sums (addresses Use of int for internal counter #31)slices.BinarySearch: Replaces manualsearchIntsimplementationImplementation Tasks
Code Changes
go.modgo.modmath/randtomath/rand/v2inttouint64PickSourcemethodsort.Slicewithslices.SortFuncsearchIntswithslices.BinarySearchPickWith(r *rand.Rand)method for explicit custom rand sourcePick()to always use global rand (no conditional)Tests & Benchmarks
PickWithto verify functionality.Documentation & Release
Release Sequence
v2branch from main (preserves v2 for maintenance)PickWithprovides non-deprecated custom rand source supportuint64for larger weight sumsmath/rand/v2with performance improvementsv3.0.0Benchmark Comparison (v2 vs v3)
Benchmarks run on Apple M4 Pro, Go 1.24, 10 iterations per benchmark.
Results