Skip to content

Commit 114eca7

Browse files
craig[bot]dhartunianspilchen
committed
151507: util/stop: update goroutine of span in Handle.Activate r=arulajmani a=dhartunian 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: dhartunian@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 ---- util/stop: add tracing case to stopper benchmarks This is helpful for measuring impact of changes when tracing is enabled. Epic: None Release note: None 151540: roachtest/asyncpg: ignore flaky TestExecuteMany test r=spilchen a=spilchen The asyncpg test ``` test_execute.TestExecuteMany.test_executemany_server_failure_during_writes ``` has been observed to be flaky. This change adds the test to the ignore list. Fixes: #151024 Epic: none Release note: none Co-authored-by: David Hartunian <[email protected]> Co-authored-by: Matt Spilchen <[email protected]>
3 parents 3086264 + c34f199 + 0590ceb commit 114eca7

File tree

5 files changed

+110
-56
lines changed

5 files changed

+110
-56
lines changed

‎pkg/cmd/roachtest/tests/asyncpg_blocklist.go‎

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -69,20 +69,21 @@ var asyncpgBlocklist = blocklist{
6969
}
7070

7171
var asyncpgIgnoreList = blocklist{
72-
`test_pool.TestPool.test_pool_01`: "can't parse output",
73-
`test_copy.TestCopyFrom.test_copy_from_query_basics`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
74-
`test_copy.TestCopyFrom.test_copy_from_query_cancellation_explicit`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
75-
`test_copy.TestCopyFrom.test_copy_from_query_timeout_1`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
76-
`test_copy.TestCopyFrom.test_copy_from_query_to_sink`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
77-
`test_copy.TestCopyFrom.test_copy_from_table_large_rows`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
78-
`test_copy.TestCopyTo.test_copy_records_to_table_1`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
79-
`test_copy.TestCopyTo.test_copy_records_to_table_no_binary_codec`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
80-
`test_copy.TestCopyTo.test_copy_to_table_basics`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
81-
`test_copy.TestCopyTo.test_copy_to_table_fail_in_source_1`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
82-
`test_copy.TestCopyTo.test_copy_to_table_fail_in_source_2`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
83-
`test_copy.TestCopyTo.test_copy_to_table_from_bytes_like`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
84-
`test_copy.TestCopyTo.test_copy_to_table_from_file_path`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
85-
`test_copy.TestCopyTo.test_copy_to_table_large_rows`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
86-
`test_copy.TestCopyTo.test_copy_to_table_timeout`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
87-
`test_listeners.TestListeners.test_dangling_listener_warns`: "flaky",
72+
`test_pool.TestPool.test_pool_01`: "can't parse output",
73+
`test_copy.TestCopyFrom.test_copy_from_query_basics`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
74+
`test_copy.TestCopyFrom.test_copy_from_query_cancellation_explicit`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
75+
`test_copy.TestCopyFrom.test_copy_from_query_timeout_1`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
76+
`test_copy.TestCopyFrom.test_copy_from_query_to_sink`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
77+
`test_copy.TestCopyFrom.test_copy_from_table_large_rows`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
78+
`test_copy.TestCopyTo.test_copy_records_to_table_1`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
79+
`test_copy.TestCopyTo.test_copy_records_to_table_no_binary_codec`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
80+
`test_copy.TestCopyTo.test_copy_to_table_basics`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
81+
`test_copy.TestCopyTo.test_copy_to_table_fail_in_source_1`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
82+
`test_copy.TestCopyTo.test_copy_to_table_fail_in_source_2`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
83+
`test_copy.TestCopyTo.test_copy_to_table_from_bytes_like`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
84+
`test_copy.TestCopyTo.test_copy_to_table_from_file_path`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
85+
`test_copy.TestCopyTo.test_copy_to_table_large_rows`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
86+
`test_copy.TestCopyTo.test_copy_to_table_timeout`: "flaky; see #119291 and https://github.com/MagicStack/asyncpg/issues/240",
87+
`test_execute.TestExecuteMany.test_executemany_server_failure_during_writes`: "flaky",
88+
`test_listeners.TestListeners.test_dangling_listener_warns`: "flaky",
8889
}

‎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_bench_test.go‎

Lines changed: 54 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@ import (
1010
"sync"
1111
"testing"
1212

13+
"github.com/cockroachdb/cockroach/pkg/testutils"
1314
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
15+
"github.com/cockroachdb/cockroach/pkg/util/tracing"
1416
)
1517

1618
func BenchmarkStopper(b *testing.B) {
@@ -19,52 +21,64 @@ func BenchmarkStopper(b *testing.B) {
1921
s := NewStopper()
2022
defer s.Stop(ctx)
2123

22-
opts := TaskOpts{
23-
TaskName: "testTask",
24-
}
24+
tracer := tracing.NewTracer()
2525

26-
b.Run("RunTask", func(b *testing.B) {
27-
b.ReportAllocs()
28-
var wg sync.WaitGroup // for fairness
29-
wg.Add(b.N)
30-
for i := 0; i < b.N; i++ {
31-
if err := s.RunTask(ctx, opts.TaskName, func(context.Context) { wg.Done() }); err != nil {
32-
b.Fatal(err)
33-
}
26+
testutils.RunTrueAndFalse(b, "tracing", func(b *testing.B, tracingEnabled bool) {
27+
ctx := context.Background()
28+
opts := TaskOpts{
29+
TaskName: "testTask",
3430
}
35-
wg.Wait() // noop
36-
})
37-
38-
b.Run("AsyncTaskEx", func(b *testing.B) {
39-
b.ReportAllocs()
40-
var wg sync.WaitGroup
41-
for i := 0; i < b.N; i++ {
42-
wg.Add(1)
43-
if err := s.RunAsyncTaskEx(ctx, opts, func(ctx context.Context) {
44-
defer wg.Done()
45-
}); err != nil {
46-
b.Fatal(err)
47-
}
48-
wg.Wait()
31+
if tracingEnabled {
32+
opts.SpanOpt = ChildSpan
33+
sp := tracer.StartSpan("test span")
34+
defer sp.Finish()
35+
ctx = tracing.ContextWithSpan(ctx, sp)
4936
}
50-
})
5137

52-
hdlf := func(ctx context.Context, hdl *Handle, wg *sync.WaitGroup) {
53-
defer hdl.Activate(ctx).Release(ctx)
54-
defer wg.Done()
55-
}
38+
b.Run("RunTask", func(b *testing.B) {
39+
b.ReportAllocs()
40+
var wg sync.WaitGroup // for fairness
41+
wg.Add(b.N)
42+
for i := 0; i < b.N; i++ {
43+
if err := s.RunTask(ctx, opts.TaskName, func(context.Context) { wg.Done() }); err != nil {
44+
b.Fatal(err)
45+
}
46+
}
47+
wg.Wait() // noop
48+
})
5649

57-
b.Run("Handle", func(b *testing.B) {
58-
b.ReportAllocs()
59-
var wg sync.WaitGroup
60-
for i := 0; i < b.N; i++ {
61-
ctx, hdl, err := s.GetHandle(ctx, opts)
62-
if err != nil {
63-
b.Fatal(err)
50+
b.Run("AsyncTaskEx", func(b *testing.B) {
51+
b.ReportAllocs()
52+
var wg sync.WaitGroup
53+
for i := 0; i < b.N; i++ {
54+
wg.Add(1)
55+
if err := s.RunAsyncTaskEx(ctx, opts, func(ctx context.Context) {
56+
defer wg.Done()
57+
}); err != nil {
58+
b.Fatal(err)
59+
}
60+
wg.Wait()
6461
}
65-
wg.Add(1)
66-
go hdlf(ctx, hdl, &wg)
67-
wg.Wait()
62+
})
63+
64+
hdlf := func(ctx context.Context, hdl *Handle, wg *sync.WaitGroup) {
65+
defer hdl.Activate(ctx).Release(ctx)
66+
defer wg.Done()
6867
}
68+
69+
b.Run("Handle", func(b *testing.B) {
70+
b.ReportAllocs()
71+
var wg sync.WaitGroup
72+
for i := 0; i < b.N; i++ {
73+
ctx, hdl, err := s.GetHandle(ctx, opts)
74+
if err != nil {
75+
b.Fatal(err)
76+
}
77+
wg.Add(1)
78+
go hdlf(ctx, hdl, &wg)
79+
wg.Wait()
80+
}
81+
})
6982
})
83+
7084
}

‎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)