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/compile: only use sequential compile queue algorithm for -v compiles, not all -c=1 compiles #46074

Open
mdempsky opened this issue May 10, 2021 · 7 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mdempsky
Copy link
Contributor

The other day I was seeing issues with "toolstash -cmp" failing because compiler output seemed non-deterministic. I submitted golang.org/cl/318229 to change non-concurrent builds to have a deterministic order, on the assumption that concurrent builds are expected to have non-deterministic output. But @josharian points out that's not the case.

However, I can no longer immediately reproduce the non-determinism issue, even with high -c values.

This is a tracking issue to check that with all the compiler refactoring this cycle we haven't broken deterministic output.

/cc @randall77 @cuonglm

@mdempsky mdempsky added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels May 10, 2021
@mdempsky mdempsky added this to the Go1.17 milestone May 10, 2021
@mdempsky mdempsky self-assigned this May 10, 2021
@mdempsky
Copy link
Contributor Author

Okay, it turns out the issue I was running into wasn't that the compiler output was non-deterministic, but just the compiler diagnostics were non-deterministic. This in turn made diffing the log files generated by toolstash -cmp useless.

CL 318229 was unnecessary for deterministic compiler output, but it does correctly make the diagnostics deterministic as needed for toolstash. I think that CL can stay for 1.17.

For 1.18, I think it might make sense to tweak this logic:

if nWorkers := base.Flag.LowerC; nWorkers > 1 {

to keep normal -c=1 builds using the same work queue algorithm as -c>1 builds, and only use the strictly in-order algorithm for -v builds or something.

@mdempsky mdempsky changed the title cmd/compile: check that we haven't regressed on deterministic compiler output cmd/compile: only use sequential compile queue algorithm for -v compiles, not all -c=1 compiles May 20, 2021
@mdempsky mdempsky modified the milestones: Go1.17, Go1.18 May 20, 2021
@mdempsky mdempsky 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 20, 2021
@josharian
Copy link
Contributor

Phew. Have you confirmed that CL 318229 doesn’t cause a performance regression?

@mdempsky
Copy link
Contributor Author

@josharian I did not, but I don't anticipate it should:

  1. It only changed the behavior of -c=1 builds. If anything, I'd expect compiling all functions on a single goroutine would be marginally faster than spinning up one goroutine per function, and then serializing them on a 1-element semaphore.
  2. It's only during the 1.17 dev cycle that we changed -c=1 and -c>1 builds to use the same algorithm. So even if CL 318229 did have any performance impact, it shouldn't be any different than Go 1.16's performance (which used essentially the same algorithm).

@josharian
Copy link
Contributor

Ah, sorry, I missed that. Thanks!

@ianlancetaylor
Copy link
Contributor

@mdempsky This is in the 1.18 milestone; time to move to 1.19? Thanks.

@mdempsky mdempsky modified the milestones: Go1.18, Go1.19 Jan 28, 2022
@cherrymui
Copy link
Member

One place I need deterministic compiling order is for debugging the compiler. Once I had a compiler bug that caused compiler crashes in multiple functions (in the same package). Once it crashed, it stopped compiling other functions. What function it crashed in was nondeterministic. I wanted it to always crash in the same function, so I can get an SSA dump and some debug outputs. I tried -c=1 but it wasn't deterministic (maybe that changed?). I think there would be good to have a way to enforce deterministic order.

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Jun 24, 2022

@mdempsky This is in the 1.19 milestone; time to move to 1.20? Or Backlog? Thanks.

@dmitshur dmitshur added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 12, 2022
@aclements aclements modified the milestones: Go1.19, Backlog Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: Triage Backlog
Status: No status
Development

No branches or pull requests

6 participants