-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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,cmd/compile: let cmd/compile choose how much concurrency to use #48497
Comments
But cmd/go also runs multiple cmd/compile processes concurrently, and we don't want them stepping on each others' toes, right? Is there a good way to have them cooperate? Or is the idea that cmd/go would set GOMAXPROCS for each subprocess based on what share of CPUs it should utilize? |
Maybe? But that's arguably kind of the kernel's job anyway — why are we doing its job for it? 😅
Not sure. In theory we could have some kind of cross-process semaphore, but in practice... that's what the kernel's scheduler is supposed to be doing. I would expect that ultimately the limiting factor on compilation concurrency would be RAM for parse trees and other per-file data structures, which really don't depend on the number of active threads — but perhaps my expectations are off somewhere. The conclusion in CL 41192 was:
However, as far as I can tell the analysis there mostly shows that increasing concurrency wouldn't help for the packages in |
A few minor observations.
Because not all of them do their job well, and it's easy for us to control concurrent access. Same answer as for why a lot of tools have semaphores guarding I/O and other syscalls.
This seems eminently sensible, and would provide a lot of simplification. +1 |
My impression was that you get the best throughput when the number worker threads is close to the number of CPU cores, and increasing the number of workers past that leads to cache thrashing. E.g., if we already have N cmd/compile processes running across N cores, it doesn't help for the cmd/compile processes to internally switch back and forth between multiple functions; it just reduces memory locality. |
That matches my intuition, but if the kernel's time-slice granularity is big enough then it shouldn't matter much either way. (For comparison, Go's garbage collector also thrashes the cache, but GCs are infrequent enough that it's not that big a deal.) |
Maybe? But if we move the choice to the (If you invoke the compiler through |
Something I wonder is the "critical path". If the toolchain is able to build 10 packages in parallel, because we have enough CPUs at hand, but one of those packages should be given priority because it is imported by another dozen of packages we also need to build - will it actually be given more CPU time earlier than the other 9? |
This is something I feel strongly about. I don't want to have to remember to set up a special env var on new machines, and I don't want to have to explain to new contributors about it, or debug what happens when they haven't. Plain |
@mvdan we don't currently have any critical path analysis, but that's a good point—cmd/go is the tool that sees the big picture, and is thus better placed to make resource allocation decisions. |
EDIT: Not relevant to discussionWould there be a GNU make jobserver compatible implementation? That would allow polyglot code-bases to be compiled without stepping on many toes. This can be an advanced option, not required for 80% of usecases. Currently, the tooling for C/C++ and rust already supports it, but golang doesn't, making the build scripts slightly sequential (not a big deal right now). I assume this would also open doors for some very neat integration with on-prem CI (eg: high utilization without page thrashing due to context switches when total processes > number of cores (difficult to control right now)) PS: am I going about it in the wrong thread? I think this is tangentially related to the topic at hand |
@kunaltyagi This discussion is about the Go tool invoking the compiler, so, yes, I think the idea of adding a GNU make jobserver implementation should be in a different issue. Thanks. |
Got it, thanks. I've created a separate issue, and hidden the previous comment to prevent derailing of further conversation |
In reviewing CL 351049, my attention was drawn to a complicated function that
cmd/go
uses to decide whether to requestcmd/compile
to use internal concurrency:go/src/cmd/go/internal/work/gc.go
Lines 213 to 285 in b1bedc0
I asked the compiler team about it, and @rsc pointed out a parallel function in
cmd/compile
, which ultimately terminates the process if the flags are not compatible:go/src/cmd/compile/internal/base/flag.go
Lines 329 to 364 in c7f2f51
Those two functions implement slightly different logic for when concurrent compilation is allowed, and I expect that they will only tend to drift more over time. Moreover, having the compiler reject values of
-N
greater than 0 when certain flags are in use makes it harder for users to overridecmd/go
s current hard-coded default for the concurrency limit (which may be a factor in #17751).The extra complexity in
cmd/go
also leads to subtle bugs like #48490.It's obviously reasonable for the compiler to use internal synchronization to avoid races and to produce deterministic output, and at the limit “dropping the concurrency to 1” is equivalent to “using internal synchronization”. So, I think:
cmd/go
should avoid setting the-c
flag explicitly at all, and instead letcmd/compile
choose its own reasonable defaults.If
cmd/compile
detects that othergcflags
are incompatible with concurrent compilation, it should drop to sequential compilation on its own. (That is, the-c
flag should specify an upper limit, not an exact one.) That way, users can setGOFLAGS=-gcflags=-c=N
and not worry about what other flags they might end up needing to pass.Ideally
cmd/compile
should choose defaults based onGOMAXPROCS
rather than a hard-coded constant, since not all packages have uniform shape (especially machine-generated API bindings!) and not all users' workstations have the same number of hardware threads.CC @josharian @randall77 @mvdan
The text was updated successfully, but these errors were encountered: