Skip to content

Commit c34f199

Browse files
committed
util/stop: update goroutine of span in Handle.Activate
The goroutine in the span of the Handle would be the goroutine of the stopper, not the task, this change explicitly updates the goroutine ID of the span when the handle is Activated. ``` ❯ benchdiff ./pkg/util/stop -b -r 'BenchmarkStopper' -d "1000x" -c 30 --preview old: 8dfc82d util/stop: add tracing case to stopper benchmarks new: 01429ca util/stop: update goroutine of span in `Handle.Act args: benchdiff "./pkg/util/stop" "-b" "-r" "BenchmarkStopper" "-d" "1000x" "-c" "30" "--preview" name old time/op new time/op delta Stopper/tracing=false/Handle-10 1.32µs ±12% 1.26µs ±12% -4.47% (p=0.003 n=29+30) Stopper/tracing=false/RunTask-10 28.0ns ±13% 27.5ns ± 6% ~ (p=0.284 n=29+26) Stopper/tracing=false/AsyncTaskEx-10 1.42µs ±14% 1.38µs ±13% ~ (p=0.081 n=30+30) Stopper/tracing=true/RunTask-10 27.8ns ±11% 27.6ns ± 9% ~ (p=0.814 n=27+28) Stopper/tracing=true/AsyncTaskEx-10 1.87µs ±13% 1.82µs ±10% ~ (p=0.186 n=30+30) Stopper/tracing=true/Handle-10 1.72µs ± 8% 1.74µs ±10% ~ (p=0.284 n=30+30) name old alloc/op new alloc/op delta Stopper/tracing=false/AsyncTaskEx-10 75.0B ± 0% 74.6B ± 2% -0.49% (p=0.016 n=20+30) Stopper/tracing=false/RunTask-10 0.00B 0.00B ~ (all equal) Stopper/tracing=false/Handle-10 50.0B ± 0% 50.0B ± 0% ~ (all equal) Stopper/tracing=true/RunTask-10 0.00B 0.00B ~ (all equal) Stopper/tracing=true/AsyncTaskEx-10 239B ± 1% 239B ± 1% ~ (p=0.431 n=30+30) Stopper/tracing=true/Handle-10 215B ± 1% 215B ± 1% ~ (p=0.315 n=30+30) name old allocs/op new allocs/op delta Stopper/tracing=false/RunTask-10 0.00 0.00 ~ (all equal) Stopper/tracing=false/AsyncTaskEx-10 3.00 ± 0% 3.00 ± 0% ~ (all equal) Stopper/tracing=false/Handle-10 1.00 ± 0% 1.00 ± 0% ~ (all equal) Stopper/tracing=true/RunTask-10 0.00 0.00 ~ (all equal) Stopper/tracing=true/AsyncTaskEx-10 4.00 ± 0% 4.00 ± 0% ~ (all equal) Stopper/tracing=true/Handle-10 2.00 ± 0% 2.00 ± 0% ~ (all equal) ``` Epic: None Release Note: None
1 parent 8dfc82d commit c34f199

File tree

3 files changed

+39
-0
lines changed

3 files changed

+39
-0
lines changed

pkg/util/stop/BUILD.bazel

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,9 @@ go_test(
4141
"//pkg/util/quotapool",
4242
"//pkg/util/syncutil",
4343
"//pkg/util/tracing",
44+
"//pkg/util/tracing/tracingpb",
4445
"@com_github_cockroachdb_errors//:errors",
46+
"@com_github_petermattis_goid//:goid",
4547
"@com_github_stretchr_testify//require",
4648
],
4749
)

pkg/util/stop/handle.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,10 @@ type ActiveHandle interface {
4646
func (hdl *Handle) Activate(ctx context.Context) ActiveHandle {
4747
growstack.Grow() // see https://github.com/cockroachdb/cockroach/issues/130663
4848

49+
// Activate is called once we're inside the new goroutine.
50+
if hdl.sp != nil {
51+
hdl.sp.UpdateGoroutineIDToCurrent()
52+
}
4953
hdl.region = hdl.s.startRegion(ctx, hdl.taskName)
5054
// NB: it's tempting for ergonomics to make `release` a method on `Handle` and
5155
// to return `hdl.release` here, but that allocates.

pkg/util/stop/stopper_test.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ package stop_test
77

88
import (
99
"context"
10+
"fmt"
1011
"runtime"
1112
"sync"
1213
"sync/atomic"
@@ -21,7 +22,9 @@ import (
2122
"github.com/cockroachdb/cockroach/pkg/util/stop"
2223
"github.com/cockroachdb/cockroach/pkg/util/syncutil"
2324
"github.com/cockroachdb/cockroach/pkg/util/tracing"
25+
"github.com/cockroachdb/cockroach/pkg/util/tracing/tracingpb"
2426
"github.com/cockroachdb/errors"
27+
"github.com/petermattis/goid"
2528
"github.com/stretchr/testify/require"
2629
)
2730

@@ -742,3 +745,33 @@ func TestHandleWithoutActivateOrRelease(t *testing.T) {
742745
}
743746
})
744747
}
748+
749+
func TestHandleSetsGoroutineOnSpan(t *testing.T) {
750+
defer leaktest.AfterTest(t)()
751+
ctx := context.Background()
752+
753+
s := stop.NewStopper()
754+
defer s.Stop(ctx)
755+
756+
tracer := tracing.NewTracer()
757+
sp := tracer.StartSpan("test span")
758+
sp.SetRecordingType(tracingpb.RecordingVerbose)
759+
ctx = tracing.ContextWithSpan(ctx, sp)
760+
761+
ctx, hdl, err := s.GetHandle(ctx, stop.TaskOpts{
762+
SpanOpt: stop.ChildSpan,
763+
TaskName: "async task",
764+
})
765+
require.NoError(t, err)
766+
767+
endCh := make(chan struct{})
768+
var goroutineID int64
769+
go func() {
770+
defer hdl.Activate(ctx).Release(ctx)
771+
goroutineID = goid.Get()
772+
close(endCh)
773+
}()
774+
<-endCh
775+
rec := sp.FinishAndGetRecording(tracingpb.RecordingVerbose)
776+
require.Contains(t, rec.String(), fmt.Sprintf("gid:%d", goroutineID))
777+
}

0 commit comments

Comments
 (0)