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.

Sign up for GitHub

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

Jump to bottom

crypto/internal/randutil: use math/rand instead of select to speed up MaybeReadByte #26178

Closed
minaevmike opened this issue Jul 2, 2018 · 4 comments
Closed

crypto/internal/randutil: use math/rand instead of select to speed up MaybeReadByte #26178

minaevmike opened this issue Jul 2, 2018 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Go1.12

Comments

@minaevmike
Copy link
Contributor

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.10.3 linux/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/mike/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/root/work/prog/GO"
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build468939385=/tmp/go-build -gno-record-gcc-switches"

https://github.com/golang/go/blob/master/src/crypto/internal/randutil/randutil.go#L31
I am not sure this is the best way to make some action with some probability. Or mb there are some reasons to it like this? I wrote a benchmark https://play.golang.org/p/XA0Uv17BPfs .
On my local machine i have such results:

$ go test -benchtime 5s -bench=. -cpu=1,2,4,8
goos: linux
goarch: amd64
BenchmarkSelectParallel     	20000000	       300 ns/op
BenchmarkSelectParallel-2   	30000000	       217 ns/op
BenchmarkSelectParallel-4   	30000000	       229 ns/op
BenchmarkSelectParallel-8   	50000000	       242 ns/op
BenchmarkRandomParallel     	100000000	        74.1 ns/op
BenchmarkRandomParallel-2   	200000000	        40.4 ns/op
BenchmarkRandomParallel-4   	100000000	        55.9 ns/op
BenchmarkRandomParallel-8   	100000000	        73.1 ns/op

I think random can show better results after using fastrand from runtime package.
@ianlancetaylor
Copy link
Contributor

We don't use the issue tracker to ask questions. Please see https://golang.org/wiki/Questions . Thanks.

But I think in this case you are suggesting a different implementation for MaybeReadByte, so retitling the issue.

@ianlancetaylor ianlancetaylor changed the title crypto/internal/randutil: perfomance question crypto/internal/randutil: use math/rand instead of select to speed up MaybeReadByte Jul 2, 2018
@ianlancetaylor ianlancetaylor added Performance NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 2, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Jul 2, 2018
@ianlancetaylor
Copy link
Contributor

CC @agl @FiloSottile

@meirf
Copy link
Contributor

meirf commented Jul 3, 2018 •
edited

@minaevmike it appears you accidentally left some code in your benchmark which makes the rand.Int() alternative seem like a 3x or 4x improvement, where it is really a ~1.5x improvement. In particular, BenchmarkSelectParallel after already running the benchmark in parallel has the following additional serial code:

for i := 0; i < b.N; i++ {
    select {
        case <-c:
		first++
	case <-c:
		second++
	}
}

After removing that, I get:

$ go test -benchtime 5s -bench=. -cpu=1,2,4,8
goos: darwin
goarch: amd64
BenchmarkSelectParallel     	100000000	        81.1 ns/op
BenchmarkSelectParallel-2   	100000000	       111 ns/op
BenchmarkSelectParallel-4   	50000000	       126 ns/op
BenchmarkSelectParallel-8   	50000000	       176 ns/op
BenchmarkRandomParallel     	200000000	        35.4 ns/op
BenchmarkRandomParallel-2   	100000000	        81.6 ns/op
BenchmarkRandomParallel-4   	50000000	       119 ns/op
BenchmarkRandomParallel-8   	50000000	       126 ns/op

rand.Int() does outperform the select in all these cases, but it's not the 4x initially thought.

(minaevmike, please let me know if I'm mistaken/that code was intentionally put there.)

@minaevmike
Copy link
Contributor Author

@meirf , yes you are right, that was my stupid mistake. I think we can close this

@golang golang locked and limited conversation to collaborators Jul 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
4 participants
@meirf @minaevmike @ianlancetaylor @gopherbot