Exploratory work towards a potential v3 release #36
+67
−139
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Key breaking changes would include (TBD list in progress):
math/rand/v2, exposing a different surface area for custom randomness sources (see Nit: undeprecate PickSource #29)Non-breaking changes:
uint64, no API changes but more easily implemented with themath/rand/v2migration. (addresses Use of int for internal counter #31)