-
Notifications
You must be signed in to change notification settings - Fork 119
Conversation
Previous time: 15.175 ns/op New time: 4.78 ns/op
30% less memory used and a 5 to 7% higher throughput.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
return true | ||
} | ||
return false | ||
return r.Int31()&(1<<30) == 0 |
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.
Why bit 31 and not some other bit?
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 had to pick one and I believe there was no better choice :)
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.
Yeah, they should all be the same. I just wasn't sure if the change from bit 0 was significant for the speed up--it shouldn't be, right?
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.
From my benchmarks, on my local machine I've found this bit to be faster from bit 0 by 1-2% - why, I don't know - probably some CPU arch black magic.
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.
hm... you can use https://godoc.org/golang.org/x/tools/cmd/benchcmp to tell if it's significant or not. (not shown in the help: you can put multiple runs in both the old and new file and it will give you the std dev so you can tell if it's significant or not)
Thanks! Can you add your benchmark, too? |
Sure!
|
Can you add the benchmarks to the _test.go file? (just one for Bool and one for String; no need for old/new--we can just track that over time) |
Added benchmarks :) |
fuzz_test.go
Outdated
rs := rand.New(rand.NewSource(123)) | ||
|
||
for i := 0; i < b.N; i++ { | ||
randBool(rs) |
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.
Looks good-- can you run gofmt?
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.
Done 😃
for i := range runes { | ||
runes[i] = unicodeRanges[r.Intn(len(unicodeRanges))].choose(r) | ||
sb := strings.Builder{} | ||
sb.Grow(n) |
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 bet n*2
or n*3
would avoid an extra allocation or two.
LGTM thanks! |
I love this library, small but gets the job done! :)
I have tinkered a bit with the functions to generate bools and strings and made them a bit faster. Hopefully it will save someone some clocks when running tests.
The code speeds up bool generation 3-fold. I've ran some benchmarks on GCP VM's and my local MacBook:
randString() will be 5-7% due to less data being moved around (thanks to strings.Builder)
randString() memory usage change: