build: add go runtime patch to bound non-preemptible work#118605
build: add go runtime patch to bound non-preemptible work#118605craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
|
This change is |
|
Also probably a good idea to get someone from security to take a look at the second commit that has the crypto package changes. |
94d6d35 to
45b0365
Compare
|
Test failure is #118520. I'll rebase. |
We currently maintain a set of patches to the Go runtime. To-date, our various patches have accumulated in a single patch file. Ideally, our set of patches should be periodically re-applied to the base of the Go runtime that we build, to ensure that our patches still apply cleanly. However, the lack of ordering in the patch currently file makes this difficult. Alphabetize the patches to avoid future diff cheese. The changes in this patch were generated using the following: ``` _patch_file="$CRDB_SRC_ROOT/build/teamcity/internal/release/build-and-publish-patched-go/diff.patch" $ cd go $ git checkout go1.21.5 $ git apply "$_patch_file" $ git diff > "$_patch_file" ``` Release note: None.
Assembly code is non-preemptible, and goroutines running such pre-emptible calls can delay stop-the-world GC pauses, impacting tail latency if they are timed poorly. Certain backup codepaths in Cockroach will spend a material amount of time encrypting large blocks of data. These calls involve non-preemptible calls. Mitigate the risk for these crypto libraries by bounding the work that any single call into an assembly routine performs. The impact of this change on the `BenchmarkLatencyWhileHashing` can be found here. In particular, for the SHA256 algorithm, the impact on latency was observed to improve by close to 2x, when encrypting larger block sizes. Inspiration taken from golang/go#/64417. Touches cockroachdb#115192. Release note: None.
45b0365 to
b018b1b
Compare
|
rsevinsky-cr
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @dt, @nvanbenschoten, and @sumeerbhola)
rsevinsky-cr
left a comment
There was a problem hiding this comment.
Reviewable status:
This commit updates the go stdlib patch from the previous commit to include testing that exercises the sha256 portion. Epic: None Release note: None
nvb
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 2 of 3 files at r2.
Reviewable status:
build/teamcity/internal/release/build-and-publish-patched-go/diff.patch line 177 at r2 (raw file):
if len(p) >= chunk { n := len(p) &^ (chunk - 1) + for n > maxAsmSize {
Without any new testing, we won't have any test coverage for the sha256 version of this logic. I've gone ahead and pushed a new commit to this PR to add testing for sha256. Since this is just testing, I won't rebuild the custom go sdk.
|
CI failure appears to just be that merge base is too old to have #118665 |
|
bors r=nvanbenschoten,rickystewart,rsevinsky-cr |
|
Build succeeded: |
|
blathers backport 23.2 |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from b018b1b to blathers/backport-release-23.2-118605: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.2 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
Yeah, figured that was going to be its answer :( |
This commit applies changes from cockroachdb#118605 into v23.2. It includes the following commits: --- build: bound work in non-preemptible assembly loops Assembly code is non-preemptible, and goroutines running such pre-emptible calls can delay stop-the-world GC pauses, impacting tail latency if they are timed poorly. Certain backup codepaths in Cockroach will spend a material amount of time encrypting large blocks of data. These calls involve non-preemptible calls. Mitigate the risk for these crypto libraries by bounding the work that any single call into an assembly routine performs. The impact of this change on the `BenchmarkLatencyWhileHashing` can be found here. In particular, for the SHA256 algorithm, the impact on latency was observed to improve by close to 2x, when encrypting larger block sizes. Inspiration taken from golang/go#/64417. --- build: add testing for crypto/sha256 patch This commit updates the go stdlib patch from the previous commit to include testing that exercises the sha256 portion. Epic: None Release note: None
This commit applies changes from cockroachdb#118605 into v23.1. It includes the following commits: --- build: bound work in non-preemptible assembly loops Assembly code is non-preemptible, and goroutines running such pre-emptible calls can delay stop-the-world GC pauses, impacting tail latency if they are timed poorly. Certain backup codepaths in Cockroach will spend a material amount of time encrypting large blocks of data. These calls involve non-preemptible calls. Mitigate the risk for these crypto libraries by bounding the work that any single call into an assembly routine performs. The impact of this change on the `BenchmarkLatencyWhileHashing` can be found here. In particular, for the SHA256 algorithm, the impact on latency was observed to improve by close to 2x, when encrypting larger block sizes. Inspiration taken from golang/go#/64417. --- build: add testing for crypto/sha256 patch This commit updates the go stdlib patch from the previous commit to include testing that exercises the sha256 portion. Epic: None Release note: None
|
Cool! |
There are two commits in this patch set, and are best reviewed individually.
The first is a purely mechanical change to reapply our existing Go runtime patches to the upstream, and then port the diff back into Cockroach. This shuffles around the ordering of the various patches, making subsequent patching simpler.
The second patch is cribbed from golang/go#64417, and is the material change, touching #115192.
Epic: None.