ci: add scripts to build patched versions of Go runtime #84867

rickystewart · 2022-07-21T18:52:17Z

The script simply uses our existing cross toolchains and applies a patch
to the Go sources for the fine-grained CPU attribution work. We make a
custom go executable for all supported platforms except FreeBSD, for
which we have no cross-compilers, and M1/M2 Macs, for which we don't
have a code-signing pipeline set up yet.

Also update build/README.MD to capture the new process of building
dependencies.

Part of #82625.

Release note: None

cockroach-teamcity · 2022-07-21T18:52:25Z

This change is

Package grunning is a library that's able to retrieve on-CPU running time for individual goroutines. It relies on using a patched Go and provides a primitive for fine-grained CPU attribution and control through a single API: package grunning // Time returns the time spent by the current goroutine in the // running state. func Time() time.Duration The motivating RFC is over at cockroachdb#82356. Informs cockroachdb#82625. We build CRDB using use the patched Go runtime for all officially supported platforms when built using Bazel (cockroachdb#84867). Engineers commonly building CRDB also use happen to use two platforms we don't use a patched Go for: - FreeBSD (we don't have cross-compilers setup), and - M1/M2 Macs (we don't have a code-signing pipeline, yet). We use '(darwin && arm64) || freebsd || !bazel' as the build tag to exclude such platforms. See cockroachdb#84867 for more details. This package tests various properties we should expect over the running time value. It does not make assertions given the CI environments we run these under (CPU-starved, lot of OS thread pre-emption, dissimilar to healthy CRDB deployments). This is also why they're skipped under stress. Still, these tests are useful to understand the properties we expect running time to have: === RUN TestEquivalentGoroutines thread=03 expected≈10.00% got= 9.98% of on-cpu time thread=06 expected≈10.00% got=10.00% of on-cpu time thread=02 expected≈10.00% got=10.01% of on-cpu time thread=10 expected≈10.00% got=10.01% of on-cpu time thread=07 expected≈10.00% got= 9.99% of on-cpu time thread=04 expected≈10.00% got= 9.99% of on-cpu time thread=09 expected≈10.00% got=10.00% of on-cpu time thread=01 expected≈10.00% got= 9.99% of on-cpu time thread=08 expected≈10.00% got=10.02% of on-cpu time thread=05 expected≈10.00% got=10.02% of on-cpu time --- PASS: TestEquivalentGoroutines (0.56s) === RUN TestProportionalGoroutines thread=01 got 1.82% of on-cpu time: expected≈ 1.00x got=1.00x thread=02 got 3.64% of on-cpu time: expected≈ 2.00x got=2.00x thread=03 got 5.47% of on-cpu time: expected≈ 3.00x got=3.00x thread=04 got 7.28% of on-cpu time: expected≈ 4.00x got=4.00x thread=05 got 9.09% of on-cpu time: expected≈ 5.00x got=4.99x thread=06 got 10.91% of on-cpu time: expected≈ 6.00x got=5.99x thread=07 got 12.73% of on-cpu time: expected≈ 7.00x got=6.99x thread=08 got 14.54% of on-cpu time: expected≈ 8.00x got=7.99x thread=09 got 16.36% of on-cpu time: expected≈ 9.00x got=8.99x thread=10 got 18.16% of on-cpu time: expected≈10.00x got=9.97x --- PASS: TestProportionalGoroutines (1.72s) === RUN TestPingPongHog pinger/ponger expected≈1.00x got=0.96x --- PASS: TestPingPongHog (0.91s) Release note: None

85850: importer: support {} format for arrays in CSV r=ecwall a=rafiss fixes #84631 Release note (sql change): Arrays can now be imported in a CSV file using the {} format, similar to COPY FROM. Importing array expressions (e.g. ARRAY[1, 2, 3]) is still supported as well. 85854: clusterversion,storage: remove 22.1 PebbleFormat version gates r=jbowens a=celiala This commit removes the following 22.1 version gates: - PebbleFormatBlockPropertyCollector - PebbleFormatSplitUserKeysMarked Cleanup was done following guidance from [21.2 cleanup](#74270 (comment)): > For the most part, if the gates were just simple if !version.IsActive { return x } or something, I just removed the block, and even if it was a little more complicated, like args = [x]; if version { args = append(args, y) }; foo(args) I still tried to mostly inline it such that it looked natural (i.e. remove that append and make it args = [x, y]). > However for just a couple more complicated cases that were referring to <21.2 versions that needed to be replaced when those were deleted, I added a placeholder clusterversion.TODOPre21_2 alias for 21.2. Replacing those calls with this alias shouldn't change their behavior -- it was already always true, since the code today should never run in a <21.2 cluster -- but means we can delete those older versions in the meantime and then the owners of these bits can decide how to update them. Partially addresses #80663 Release note: none 85909: kvserver: instrument RaftTransport workers with pprof labels r=tbg,erikgrinaker a=pavelkalinnikov The unused arguments in the method signature were used to identify goroutines in traces. This no longer works after Go 1.17 started passing arguments via registers. This commit adds pprof labels when starting these goroutines, to have a cleaner code, more readable traces, and to work around the new Go convention. Release note: None 85977: grunning: add library for precise on-CPU time measurement r=irfansharif a=irfansharif Package grunning is a library that's able to retrieve on-CPU running time for individual goroutines. It relies on using a patched Go and provides a primitive for fine-grained CPU attribution and control through a single API: package grunning // Time returns the time spent by the current goroutine in the // running state. func Time() time.Duration The motivating RFC is over at #82356. Informs #82625. We build CRDB using use the patched Go runtime for all officially supported platforms when built using Bazel (#84867). Engineers commonly building CRDB also use happen to use two platforms we don't use a patched Go for: - FreeBSD (we don't have cross-compilers setup), and - M1/M2 Macs (we don't have a code-signing pipeline, yet). We use '(darwin && arm64) || freebsd || !bazel' as the build tag to exclude such platforms. See #84867 for more details. This package tests various properties we should expect over the running time value. It does not make assertions given the CI environments we run these under (CPU-starved, lot of OS thread pre-emption, dissimilar to healthy CRDB deployments). This is also why they're skipped under stress. Still, these tests are useful to understand the properties we expect running time to have: === RUN TestEquivalentGoroutines thread=03 expected≈10.00% got= 9.98% of on-cpu time thread=06 expected≈10.00% got=10.00% of on-cpu time thread=02 expected≈10.00% got=10.01% of on-cpu time thread=10 expected≈10.00% got=10.01% of on-cpu time thread=07 expected≈10.00% got= 9.99% of on-cpu time thread=04 expected≈10.00% got= 9.99% of on-cpu time thread=09 expected≈10.00% got=10.00% of on-cpu time thread=01 expected≈10.00% got= 9.99% of on-cpu time thread=08 expected≈10.00% got=10.02% of on-cpu time thread=05 expected≈10.00% got=10.02% of on-cpu time --- PASS: TestEquivalentGoroutines (0.56s) === RUN TestProportionalGoroutines thread=01 got 1.82% of on-cpu time: expected≈ 1.00x got=1.00x thread=02 got 3.64% of on-cpu time: expected≈ 2.00x got=2.00x thread=03 got 5.47% of on-cpu time: expected≈ 3.00x got=3.00x thread=04 got 7.28% of on-cpu time: expected≈ 4.00x got=4.00x thread=05 got 9.09% of on-cpu time: expected≈ 5.00x got=4.99x thread=06 got 10.91% of on-cpu time: expected≈ 6.00x got=5.99x thread=07 got 12.73% of on-cpu time: expected≈ 7.00x got=6.99x thread=08 got 14.54% of on-cpu time: expected≈ 8.00x got=7.99x thread=09 got 16.36% of on-cpu time: expected≈ 9.00x got=8.99x thread=10 got 18.16% of on-cpu time: expected≈10.00x got=9.97x --- PASS: TestProportionalGoroutines (1.72s) === RUN TestPingPongHog pinger/ponger expected≈1.00x got=0.96x --- PASS: TestPingPongHog (0.91s) Release note: None 85987: sql: fix aggregation of statistics r=maryliag a=maryliag Previously, because we were using a join, we were double counting statistics when we had the same fingerprint in memory and persisted. This commit adds a `DISTINCT` so we only count them once. Fixes #85958 Release note: None 86008: sql: logic test to inform maximum builtin function oid change r=chengxiong-ruan a=chengxiong-ruan This commit adds a logic test to let engineers who added a new builtin function know that the new builtin function is constructed earlier than some existing builtin functions at init time. Release note: None Co-authored-by: Rafi Shamim <[email protected]> Co-authored-by: Celia La <[email protected]> Co-authored-by: Pavel Kalinnikov <[email protected]> Co-authored-by: irfan sharif <[email protected]> Co-authored-by: Marylia Gutierrez <[email protected]> Co-authored-by: Chengxiong Ruan <[email protected]>

The `loopvarcapture` linter is part of `roachvet`. It flags incorrect uses of loop variables captured by reference in Go routines or defer statements--a common source of data races. In addition to checking Go routines created via the `go` keyword, the linter also checks against other (internal) APIs, which are known to create Go routines; this is specified via `GoRoutineFunctions`. An existing bug prevented the linter from checking inside `Monitor.Go`, which are commonly used inside roachtests. This PR fixes the linter bug as well as the pre-existing bug inside a roachtest, which this linter now detects. This PR also removes the uncessary hack, i.e., `RunDespiteErrors`, introduced in [1]. By allowing the type checker to swallow errors, the test was susceptible to silent errors, e.g., buggy testdata. Instead, we work around by disabling CGO. Epic: none Fixes: cockroachdb#102678 Release note: None [1] cockroachdb#84867 (comment)

102679: linter: loopvarcapture should check interface methods r=renatolabs,herkolategan a=srosenberg The `loopvarcapture` linter is part of `roachvet`. It flags incorrect uses of loop variables captured by reference in Go routines or defer statements--a common source of data races. In addition to checking Go routines created via the `go` keyword, the linter also checks against other (internal) APIs, which are known to create Go routines; this is specified via `GoRoutineFunctions`. An existing bug prevented the linter from checking inside `Monitor.Go`, which are commonly used inside roachtests. This PR fixes the linter bug as well as the pre-existing bug inside a roachtest, which this linter now detects. This PR also removes the uncessary hack, i.e., `RunDespiteErrors`, introduced in [1]. By allowing the type checker to swallow errors, the test was susceptible to silent errors, e.g., buggy testdata. Instead, we work around by disabling CGO. Epic: none Fixes: #102678 Release note: None [1] #84867 (comment) Co-authored-by: Stan Rosenberg <[email protected]>

The `loopvarcapture` linter is part of `roachvet`. It flags incorrect uses of loop variables captured by reference in Go routines or defer statements--a common source of data races. In addition to checking Go routines created via the `go` keyword, the linter also checks against other (internal) APIs, which are known to create Go routines; this is specified via `GoRoutineFunctions`. An existing bug prevented the linter from checking inside `Monitor.Go`, which are commonly used inside roachtests. This PR fixes the linter bug as well as the pre-existing bug inside a roachtest, which this linter now detects. This PR also removes the uncessary hack, i.e., `RunDespiteErrors`, introduced in [1]. By allowing the type checker to swallow errors, the test was susceptible to silent errors, e.g., buggy testdata. Instead, we work around by disabling CGO. Epic: none Fixes: #102678 Release note: None [1] #84867 (comment)

rickystewart added the do-not-merge bors won't merge a PR with this label. label Jul 21, 2022

rickystewart force-pushed the patchedgo branch 20 times, most recently from bbd59a5 to 8da42a3 Compare July 22, 2022 17:01

rickystewart requested review from irfansharif and rail July 22, 2022 17:01

rickystewart marked this pull request as ready for review July 22, 2022 17:02

rickystewart requested a review from a team July 22, 2022 17:02

rickystewart requested review from a team as code owners July 22, 2022 17:02

rickystewart requested review from a team July 22, 2022 17:02

irfansharif mentioned this pull request Aug 11, 2022

grunning: add library for precise on-CPU time measurement #85977

Merged

srosenberg mentioned this pull request May 1, 2023

linter: loopvarcapture should check interface methods #102679

Merged

blathers-crl bot mentioned this pull request May 11, 2023

release-23.1: linter: loopvarcapture should check interface methods #103088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: add scripts to build patched versions of Go runtime #84867

ci: add scripts to build patched versions of Go runtime #84867

rickystewart commented Jul 21, 2022 •

edited

Loading

cockroach-teamcity commented Jul 21, 2022

ci: add scripts to build patched versions of Go runtime #84867

ci: add scripts to build patched versions of Go runtime #84867

Conversation

rickystewart commented Jul 21, 2022 • edited Loading

cockroach-teamcity commented Jul 21, 2022

rickystewart commented Jul 21, 2022 •

edited

Loading