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: fails to perform concurrent compilation on darwin/arm64 #48496

Closed
cuonglm opened this issue Sep 20, 2021 · 6 comments
Closed

cmd/go: fails to perform concurrent compilation on darwin/arm64 #48496

cuonglm opened this issue Sep 20, 2021 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. ToolSpeed
Milestone
Go1.18

Comments

@cuonglm
Copy link
Member

cuonglm commented Sep 20, 2021 •
edited

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

$ go version
go version devel go1.18-9e60c37147 Mon Sep 20 16:55:47 2021 +0000 darwin/arm64

Does this issue reproduce with the latest release?

Yes, both go1.16.8 and go1.17.1

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

go env Output
$ go env

What did you do?

go build -n -a fmt 2>&1 | grep -c -- -c=

What did you expect to see?

grep -c -- -c= should return non-zero.

What did you see instead?

$ go1.16.8 build -n -a fmt 2>&1 | grep -c -- -c=
0
$ go1.17.1 build -n -a fmt 2>&1 | grep -c -- -c=
0
$ go build -n -a fmt 2>&1 | grep -c -- -c=  # Even with the fix from #48490
0

The problem seems that with arm64, there's -shared passed to the compiler:

go/src/cmd/go/internal/work/init.go

Line 173 in 79159f2

case "arm64":

cause gcBackendConcurrency return 1:

go/src/cmd/go/internal/work/gc.go

Line 214 in 79159f2

func gcBackendConcurrency(gcflags []string) int { 

$ GOOS=linux go1.17.1 build -n -a -x fmt 2>&1 | grep -c -- -c=
35
@cuonglm cuonglm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 20, 2021
@cuonglm cuonglm mentioned this issue Sep 20, 2021
Open
@bcmills bcmills added this to the Backlog milestone Sep 20, 2021
@cuonglm cuonglm mentioned this issue Oct 4, 2021
Closed
@josharian josharian mentioned this issue Oct 4, 2021
Open
@josharian
Copy link
Contributor

I've manually reviewed all uses of ctxt.Flag_shared and they all look concurrency-safe to me. And I manually tested with the race detector and found no issues. I'm going to send a CL removing that particular check.

@gopherbot
Copy link

Change https://golang.org/cl/353871 mentions this issue: cmd/go,cmd/compile: re-enable concurrent compilation on darwin/arm64

@andig
Copy link
Contributor

andig commented Oct 4, 2021

Funny enough the M1 shows all 4 fat cores at 100% when compiling gotip.

@josharian
Copy link
Contributor

@andig that probably due to process-level concurrency.

@andig
Copy link
Contributor

andig commented Oct 4, 2021 •
edited

Maybe. For what its worth: recompiling gotip with this CL applied does not provide a speedup afaikt:

❯ time GOROOT_BOOTSTRAP=/Users/andig/htdocs/golang ./make.bash
Building Go cmd/dist using /Users/andig/htdocs/golang. (devel go1.18-579ff8b131 Mon Oct 4 19:51:37 2021 +0000 darwin/arm64)
Building Go toolchain1 using /Users/andig/htdocs/golang.
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
Building Go toolchain2 using go_bootstrap and Go toolchain1.
Building Go toolchain3 using go_bootstrap and Go toolchain2.
Building packages and commands for darwin/arm64.
---
Installed Go for darwin/arm64 in /Users/andig/htdocs/golang2
Installed commands in /Users/andig/htdocs/golang2/bin
GOROOT_BOOTSTRAP=/Users/andig/htdocs/golang ./make.bash  166.35s user 19.85s system 421% cpu 44.204 total

but:

❯ ~/htdocs/golang/bin/go build -n -a fmt 2>&1 | grep -c -- -c=
37

@josharian
Copy link
Contributor

Yes, make.bash has ample process level parallelism. That is not true of all build graphs.

josharian added a commit to tailscale/go that referenced this issue Oct 7, 2021
@josharian
cmd/go,cmd/compile: re-enable concurrent compilation on darwin/arm64
399f0f1 
I've manually reviewed all uses of ctxt.Flag_shared for concurrency safety.
And I manually tested with the race detector and found no issues.
Allow -shared to be used with compiler concurrency,
thereby re-enabling concurrent compilation on darwin/arm64.

Fixes golang#48496

Change-Id: I8a084cb08e6050950e404ceb9bd7e3a20e07e9c5
Reviewed-on: https://go-review.googlesource.com/c/go/+/353871
Trust: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
(cherry picked from commit 579ff8b)
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 14, 2022
@dmitshur dmitshur modified the milestones: Backlog, Go1.18 May 14, 2022
@golang golang locked and limited conversation to collaborators May 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. ToolSpeed
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests
6 participants
@josharian @andig @dmitshur @cuonglm @bcmills @gopherbot