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

Jump to bottom

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
Labels
fuzz Issues related to native fuzzing support NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@klauspost
Copy link
Contributor

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 fuzz Issues related to native fuzzing support NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 7, 2021
@katiehockman katiehockman added this to the Unreleased milestone Jun 7, 2021
@katiehockman
Copy link
Contributor

/cc @jayconrod

@rolandshoemaker
Copy link
Member

rolandshoemaker commented Jun 7, 2021 •
edited

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

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 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 commented Jun 8, 2021 •
edited

@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 commented Jun 13, 2021 •
edited

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 katiehockman mentioned this issue Jul 14, 2021
Closed
22 tasks
@katiehockman katiehockman mentioned this issue Sep 9, 2021
Closed
@katiehockman katiehockman modified the milestones: Unreleased, Go1.18 Sep 14, 2021
@katiehockman katiehockman added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 14, 2021
@katiehockman
Copy link
Contributor

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 added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Sep 15, 2021
@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

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

@gopherbot
Copy link

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

@katiehockman katiehockman added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed release-blocker NeedsFix The path to resolution is known, but the work has not been done. labels Sep 21, 2021
@katiehockman katiehockman modified the milestones: Go1.18, Backlog Sep 21, 2021
@katiehockman
Copy link
Contributor

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
testing, cmd/go: clarify documentation
cc85bd0 
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
Assignees
No one assigned
Labels
fuzz Issues related to native fuzzing support NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
Go Security
Status: No status
Milestone
Backlog
Development

No branches or pull requests
5 participants
@arp242 @rolandshoemaker @klauspost @gopherbot @katiehockman