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

x/build: create builder that runs with GOEXPERIMENT set to 'staticlockranking' (once lock ranking change is in) #37937

Closed
danscales opened this issue Mar 18, 2020 · 11 comments

Comments

@danscales
Copy link

@danscales danscales commented Mar 18, 2020

Once my changes https://go-review.googlesource.com/c/go/+/222925 (enable build tag corresponding to GOEXPERIMENT value) and https://go-review.googlesource.com/c/go/+/207619 (enforce static lock ranking in the runtime when GOEXPERIMENT enabled) are checked in, we will want to enable a builder (probably just for linux/amd64) that runs with GOEXPERIMENT equal to staticlockranking.

This will not only test that the lock ranking is not being violated in the runtime (hence helping to avoid deadlocks related to lock acquisition ordering), but will also test that the buildtags are correctly set for a different GOEXPERIMENT value.

@danscales danscales added this to the Gccgo milestone Mar 18, 2020
@gopherbot gopherbot added the Builders label Mar 18, 2020
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Mar 18, 2020

@dmitshur dmitshur self-assigned this Mar 19, 2020
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Mar 19, 2020

@danscales Do you think just a post-submit builder (not a part of the default trybot set) is sufficient, or do you think including this builder in trybots would be helpful too?

@danscales
Copy link
Author

@danscales danscales commented Mar 19, 2020

Yes, I think a post-submit builder will be fine. This static lock ranking will only affect people who do fairly significant changes to the Go runtime, and those folks will hopefully also know when it will be useful to run the static lock ranking checks before checkin (because they changed the way certain runtime locks are used).

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Mar 19, 2020

I see. For those CLs, it'll be possible to run the new builder via SlowBots by asking for that builder specifically with something like TRY=linux-amd64-staticlockranking.

I understand it should test only the main Go repository. Let me know if think it would be helpful to test some/many/all golang.org/x repos as well.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 19, 2020

Change https://golang.org/cl/224078 mentions this issue: dashboard: add linux-amd64-staticlockranking builder

@danscales
Copy link
Author

@danscales danscales commented Mar 19, 2020

I see. For those CLs, it'll be possible to run the new builder via SlowBots by asking for that builder specifically with something like TRY=linux-amd64-staticlockranking.

I understand it should test only the main Go repository. Let me know if think it would be helpful to test some/many/all golang.org/x repos as well.

No, there will be no need to run such a builder for any other repo other than the main Go repo.

Thanks for figuring out the necessary builder work!

I hope to get my first change (222925) submitted today or tomorrow, and then it may be a few more days before I get the full 'static lock ranking' change (207619) submitted.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Mar 19, 2020

Sounds good.

Thinking about it more, it should be useful to get the builder up and running starting with the latest existing commit on master, and ensure it's passing (or failing, if setting GOEXPERIMENT to a not-yet-supported value causes an explicit failure). Then, when you're ready to land your changes, you can use the builder via SlowBots to confirm everything is working as expected (your changes and the builder).

I'll update the CL to remove the DNS.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Mar 20, 2020

CL 224078 is deployed now, and the linux-amd64-staticlockranking builder has started running. It is currently failing, as expected, because the experiment hasn't landed yet:

unknown experiment staticlockranking

(Source: https://build.golang.org/log/afcc33b62c57fa7eee7a3807a81d081a8c9eff28)

@danscales Please feel free to test your change with the new builder via SlowBots when it's ready, and let me know if anything more needs to be adjusted on the side of the builder.

/cc @andybons

@danscales
Copy link
Author

@danscales danscales commented Mar 20, 2020

OK, thank you for doing all this, @dmitshur . Will let you know how it goes with the SlowBots when I'm ready.

gopherbot pushed a commit that referenced this issue Mar 23, 2020
For each experiment that has been enabled in the toolchain, define a build tag
with the same name (but prefixed by "goexperiment.") which can be used for
compiling alternative files for the experiment. This allows changes for the
experiment, like extra struct fields in the runtime, without affecting the base
non-experiment code at all.

I use this capability in my CL for static lock ranking
(https://go-review.googlesource.com/c/go/+/207619), so that static lock ranking
can be fully enabled as a GOEXPERIMENT, but there is no overhead in the runtime
when the experiment is not enabled.

I added a test in cmd/go/testdata/scripts to make sure the build tags are being
defined properly. In order to implement the test, I needed to provide environment
variable GOEXPSTRING to the test scripts (with its value set from
objabi.Expstring(), so that it can determine the experiments baked into the
toolchain.

I filed #37937 to make a builder with
GOEXPERIMENT set to 'staticlockranking'. This builder will ensure another variant
of GOEXPERIMENT is being tested regularly for this change, as well as checking
static lock ranking in the runtime.

Change-Id: Ieb4b86107238febd105558c1e639d30cfe57ab5c
Reviewed-on: https://go-review.googlesource.com/c/go/+/222925
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 6, 2020

Change https://golang.org/cl/227379 mentions this issue: dashboard, app/appengine: set a known issue for staticlockranking builder

gopherbot pushed a commit to golang/build that referenced this issue Apr 7, 2020
…lder

The linux-amd64-staticlockranking builder was added pre-emptively so
that it could be tested and used during the development of a CL that
implements a new feature. It's expected to fail until that work is
completed. Mark it as a builder with a known issue, golang/go#38029.

The new BuildConfig.KnownIssue field can be used by builders being
added in the future so that if they fail at first, it doesn't take
away time for people looking at build.golang.org, which we always
aim to have as green as possible¹.

¹ https://groups.google.com/d/msg/golang-dev/y0yM_Xi665Q/hUpBiUPiCAAJ

Fixes golang/go#38283
For golang/go#38029
For golang/go#37937
For golang/go#11811

Change-Id: Iba1606c7021ffa7e67ddbaae2fc6b65f4e46ab34
Reviewed-on: https://go-review.googlesource.com/c/build/+/227379
Reviewed-by: Alexander Rakoczy <alex@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 8, 2020

Change https://golang.org/cl/227544 mentions this issue: dashboard: unset known issue for staticlockranking builder

gopherbot pushed a commit to golang/build that referenced this issue Apr 8, 2020
CL 207619 has been submitted to master, fixing golang.org/issue/38029.
The linux-amd64-staticlockranking builder has been passing since then.
Unset its known issue so that if there are regressions in the future,
they can be investigated.

For golang/go#38029.
For golang/go#37937.

Change-Id: I0de8e4ac122effee56aac73c741fab41512ff0c2
Reviewed-on: https://go-review.googlesource.com/c/build/+/227544
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dan Scales <danscales@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
4 participants
You can’t perform that action at this time.