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

cmd/go: respect -cpu=n on workers #46629

Open
klauspost opened this issue Jun 7, 2021 · 10 comments
Open

cmd/go: respect -cpu=n on workers #46629

klauspost opened this issue Jun 7, 2021 · 10 comments

Comments

@klauspost
Copy link
Contributor

@klauspost klauspost commented Jun 7, 2021

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

λ gotip version
go version devel go1.17-5542c10fbf Fri Jun 4 03:07:33 2021 +0000 windows/amd64

Does this issue reproduce with the latest release?

Tip branch build. Fuzzing not in latest release.

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

go env Output
λ gotip env                                                                                                                                  
set GO111MODULE=                                                                                                                             
set GOARCH=amd64                                                                                                                             
set GOBIN=                                                                                                                                   
set GOCACHE=d:\temp\gocache                                                                                                                  
set GOENV=C:\Users\klaus\AppData\Roaming\go\env                                                                                              
set GOEXE=.exe                                                                                                                               
set GOFLAGS=                                                                                                                                 
set GOHOSTARCH=amd64                                                                                                                         
set GOHOSTOS=windows                                                                                                                         
set GOINSECURE=                                                                                                                              
set GOMODCACHE=e:\gopath\pkg\mod                                                                                                             
set GONOPROXY=                                                                                                                               
set GONOSUMDB=                                                                                                                               
set GOOS=windows                                                                                                                             
set GOPATH=e:\gopath                                                                                                                         
set GOPRIVATE=                                                                                                                               
set GOPROXY=https://goproxy.io                                                                                                               
set GOROOT=C:\Users\klaus\sdk\gotip                                                                                                          
set GOSUMDB=sum.golang.org                                                                                                                   
set GOTMPDIR=                                                                                                                                
set GOTOOLDIR=C:\Users\klaus\sdk\gotip\pkg\tool\windows_amd64                                                                                
set GOVCS=                                                                                                                                   
set GOVERSION=devel go1.17-5542c10fbf Fri Jun 4 03:07:33 2021 +0000                                                                          
set GCCGO=gccgo                                                                                                                              
set AR=ar                                                                                                                                    
set CC=gcc                                                                                                                                   
set CXX=g++                                                                                                                                  
set CGO_ENABLED=1                                                                                                                            
set GOMOD=NUL                                                                                                                                
set CGO_CFLAGS=-g -O2                                                                                                                        
set CGO_CPPFLAGS=                                                                                                                            
set CGO_CXXFLAGS=-g -O2                                                                                                                      
set CGO_FFLAGS=-g -O2                                                                                                                        
set CGO_LDFLAGS=-g -O2                                                                                                                       
set PKG_CONFIG=pkg-config                                                                                                                    
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=d:\temp\wintemp\go-build102784969=/tmp/go-build -gno-record-gcc-switches 

What did you do?

Run fuzz test which has a concurrent component, or pretty much any fuzz test.

with λ gotip test -fuzz=. -test.run=None -parallel=16 -cpu=2.

What did you expect to see?

I would expect each worker to have runtime.GOMAXPROCS(2) similar to how tests and benchmarks behave.

Arguments could be made for workers defaulting to runtime.GOMAXPROCS(1), but I would at least like the option to change it easily.

What did you see instead?

Each worker has default GOMAXPROCS, leading to significant CPU overprovisioning and workers fighting for CPU threads.

@katiehockman katiehockman changed the title [dev.fuzz] Respect -cpu=n on workers. [dev.fuzz] cmd/go: respect -cpu=n on workers. Jun 7, 2021
@katiehockman katiehockman added this to the Unreleased milestone Jun 7, 2021
@katiehockman
Copy link
Member

@katiehockman katiehockman commented Jun 7, 2021

@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Jun 7, 2021

Each worker is essentially single threaded, but I guess if the code being fuzzed is doing parallelized work it could lead to resource contention. Based on my experience I've only seen a drop in performance when I set -parallel > the number of available threads.

We could default each worker process to use runtime.NumCPU()/parallel CPUs. Allowing the user to set their own limit seems reasonable.

@klauspost
Copy link
Contributor Author

@klauspost klauspost commented Jun 8, 2021

We could default each worker process to use runtime.NumCPU()/parallel CPUs. Allowing the user to set their own limit seems reasonable.

@rolandshoemaker parallel makes no sense, that is mixing apples and oranges.

It currently uses the default, which I argue also doesn't make much sense, or at least should be controllable by the existing -cpu parameter on tests, so you can run fuzz tests with different GOMAXPROCS settings.

@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Jun 8, 2021

@rolandshoemaker parallel makes no sense, that is mixing apples and oranges.

Why not? parallel controls the number of workers. If we want to prevent significant thread contention by default we can set each worker to use number of threads / number of workers threads. This should be user tunable, but we also need to set a reasonable default.

@klauspost
Copy link
Contributor Author

@klauspost klauspost commented Jun 8, 2021

@rolandshoemaker I misread your / as "or" (language meaning), not "divided by".

In my view -parallel controls the number of worker processes spawned, and has little to do with the concurrency I want to have for each.

While I understand your intent, we already have a parameter that controls concurrency, so when I use go test -cpu=4 I know my tests will be run with GOMAXPROCS(4). Your proposal gives GOMAXPROCS(1) with default settings, which is very reasonable, though TBH it seems a bit to magic for me.

@arp242
Copy link

@arp242 arp242 commented Jun 13, 2021

A documentation update to the -cpu flag would improve things IMHO; the -cpu doesn't mention it, and while it's documented in -parallel it didn't occur to me to check there, and even when reading the docs for -cpu and -parallel it isn't super-clear how they interact when fuzzing. The only reason I figured this out is because I found this issue. And/or mentioning it in help fuzz probably would be a bad idea either.

I can see how it makes sense to use -parallel, but I agree with Klaus that this is counter-intuitive. I think a lot of people will run in to this. Better documentation should probably fix most of the confusion.

@katiehockman
Copy link
Member

@katiehockman katiehockman commented Sep 15, 2021

For now, we are going to disable -cpu while fuzzing (i.e. if -fuzz is set): it'll be a no-op. We will document that if folks want to adjust this, they can just set GOMAXPROCS in their environment. Maybe in future releases we can do more.

@katiehockman katiehockman changed the title [dev.fuzz] cmd/go: respect -cpu=n on workers. [dev.fuzz] cmd/go: respect -cpu=n on workers Sep 16, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 20, 2021

Change https://golang.org/cl/350154 mentions this issue: [dev.fuzz] testing, cmd/go: clarify documentation

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 20, 2021

Change https://golang.org/cl/351113 mentions this issue: testing, cmd/go: clarify documentation

@katiehockman katiehockman removed this from the Go1.18 milestone Sep 21, 2021
@katiehockman katiehockman added this to the Backlog milestone Sep 21, 2021
@katiehockman
Copy link
Member

@katiehockman katiehockman commented Sep 21, 2021

Now that the documentation updates are in, I removed the release-blocker label and we can use this issue as discussion for future changes that may be made to support -cpu.

@rsc rsc changed the title [dev.fuzz] cmd/go: respect -cpu=n on workers cmd/go: respect -cpu=n on workers Sep 21, 2021
gopherbot pushed a commit that referenced this issue Sep 21, 2021
This CL removes 'go help fuzz' but expands the testing package
documentation with much of the same information. It also removes
documentation for the unimplemented -keepfuzzing flag and makes a
number of other clarifications.

Addressing comments in CL 348469.

Updates #46629

Change-Id: I12ab5971c900c2e43f2c2d83c6705e8cd642388b
Reviewed-on: https://go-review.googlesource.com/c/go/+/351113
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Trust: Katie Hockman <katie@golang.org>
Trust: Bryan C. Mills <bcmills@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants