internal/trace/v2: redefine NoTask and add BackgroundTask

The v2 trace parser currently handles task inheritance and region task
association incorrectly. It assumes that a TaskID of 0 means that there
is no task. However, this is only true for task events. A TaskID of 0
means that a region gets assigned to the "background task." The parser
currently has no concept of a "background task."

Fix this by defining the background task as task ID 0 and redefining
NoTask to ^uint64(0). This aligns the TaskID values more closely with
other IDs in the parser and also enables disambiguating these two cases.

For #60773.
For #63960.

Change-Id: I09c8217b33b87c8f8f8ea3b0203ed83fd3b61e11
Reviewed-on: https://go-review.googlesource.com/c/go/+/543019
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
TryBot-Bypass: Michael Knyszek <mknyszek@google.com>

diff --git a/src/internal/trace/goroutinesv2.go b/src/internal/trace/goroutinesv2.go
index c5e5fadc0bc736889ed95155c087597f8b2a13f7..65856fb80e039117d0d187f66502e07a262e8487 100644 (file)
--- a/src/internal/trace/goroutinesv2.go
+++ b/src/internal/trace/goroutinesv2.go
@@ -299,9 +299,7 @@ func (s *GoroutineSummarizer) Event(ev *tracev2.Event) {
                                if creatorG := s.gs[ev.Goroutine()]; creatorG != nil && len(creatorG.activeRegions) > 0 {
                                        regions := creatorG.activeRegions
                                        s := regions[len(regions)-1]
-                                       if s.TaskID != tracev2.NoTask {
-                                               g.activeRegions = []*UserRegionSummary{{TaskID: s.TaskID, Start: ev}}
-                                       }
+                                       g.activeRegions = []*UserRegionSummary{{TaskID: s.TaskID, Start: ev}}
                                }
                                s.gs[g.ID] = g
                        case tracev2.GoRunning:

diff --git a/src/internal/trace/v2/event.go b/src/internal/trace/v2/event.go
index 3700cbcc2f8775da4cd5635b8ea6ffa925963161..763313c3326943b5432b83b7330ccceda87a9c8c 100644 (file)
--- a/src/internal/trace/v2/event.go
+++ b/src/internal/trace/v2/event.go
@@ -203,8 +203,14 @@ type RangeAttribute struct {
 // are of the same type).
 type TaskID uint64
 
-// NoTask indicates the lack of a task.
-const NoTask = TaskID(0)
+const (
+       // NoTask indicates the lack of a task.
+       NoTask = TaskID(^uint64(0))
+
+       // BackgroundTask is the global task that events are attached to if there was
+       // no other task in the context at the point the event was emitted.
+       BackgroundTask = TaskID(0)
+)
 
 // Task provides details about a Task event.
 type Task struct {

diff --git a/src/internal/trace/v2/order.go b/src/internal/trace/v2/order.go
index 8b503d4dc406e1b2c67e03e1f815f8071952af1f..531b45eb02a56ecc3787fc0a628a1b025472f12a 100644 (file)
--- a/src/internal/trace/v2/order.go
+++ b/src/internal/trace/v2/order.go
@@ -525,6 +525,13 @@ func (o *ordering) advance(ev *baseEvent, evt *evTable, m ThreadID, gen uint64)
                // Get the parent ID, but don't validate it. There's no guarantee
                // we actually have information on whether it's active.
                parentID := TaskID(ev.args[1])
+               if parentID == BackgroundTask {
+                       // Note: a value of 0 here actually means no parent, *not* the
+                       // background task. Automatic background task attachment only
+                       // applies to regions.
+                       parentID = NoTask
+                       ev.args[1] = uint64(NoTask)
+               }
 
                // Validate the name and record it. We'll need to pass it through to
                // EvUserTaskEnd.

diff --git a/src/internal/trace/v2/testdata/generators/go122-task-across-generations.go b/src/internal/trace/v2/testdata/generators/go122-task-across-generations.go
index 94e993399611ed08da8b94b348a6f27e5e85a47e..06ef96e51a235832e87cc50dfd8b69f3da73eda9 100644 (file)
--- a/src/internal/trace/v2/testdata/generators/go122-task-across-generations.go
+++ b/src/internal/trace/v2/testdata/generators/go122-task-across-generations.go
@@ -29,7 +29,7 @@ func gen(t *testgen.Trace) {
        b1 := g1.Batch(trace.ThreadID(0), 0)
        b1.Event("ProcStatus", trace.ProcID(0), go122.ProcRunning)
        b1.Event("GoStatus", trace.GoID(1), trace.ThreadID(0), go122.GoRunning)
-       b1.Event("UserTaskBegin", trace.TaskID(2), trace.NoTask, "my task", testgen.NoStack)
+       b1.Event("UserTaskBegin", trace.TaskID(2), trace.TaskID(0) /* 0 means no parent, not background */, "my task", testgen.NoStack)
 
        g2 := t.Generation(2)
 

diff --git a/src/internal/trace/v2/testtrace/validation.go b/src/internal/trace/v2/testtrace/validation.go
index a2654a10e4e8b7c2b06e3d972fcd2c10a1a0aa2d..448ef9d9ddcfeea8fc7d03f4560f81a90fb5010e 100644 (file)
--- a/src/internal/trace/v2/testtrace/validation.go
+++ b/src/internal/trace/v2/testtrace/validation.go
@@ -256,9 +256,14 @@ func (v *Validator) Event(ev trace.Event) error {
        case trace.EventTaskBegin:
                // Validate task begin.
                t := ev.Task()
-               if t.ID == trace.NoTask {
+               if t.ID == trace.NoTask || t.ID == trace.BackgroundTask {
+                       // The background task should never have an event emitted for it.
                        e.Errorf("found invalid task ID for task of type %s", t.Type)
                }
+               if t.Parent == trace.BackgroundTask {
+                       // It's not possible for a task to be a subtask of the background task.
+                       e.Errorf("found background task as the parent for task of type %s", t.Type)
+               }
                // N.B. Don't check the task type. Empty string is a valid task type.
                v.tasks[t.ID] = t.Type
        case trace.EventTaskEnd:

diff --git a/src/internal/trace/v2/trace_test.go b/src/internal/trace/v2/trace_test.go
index 7823b01e9306fc02c61ab2b5feb1cc2d782d1205..b2d7781991863842988b51ccf43ebc7243b9cf47 100644 (file)
--- a/src/internal/trace/v2/trace_test.go
+++ b/src/internal/trace/v2/trace_test.go
@@ -35,7 +35,7 @@ func TestTraceAnnotations(t *testing.T) {
                        {trace.EventRegionEnd, trace.TaskID(1), []string{"region0"}},
                        {trace.EventTaskEnd, trace.TaskID(1), []string{"task0"}},
                        //  Currently, pre-existing region is not recorded to avoid allocations.
-                       {trace.EventRegionBegin, trace.NoTask, []string{"post-existing region"}},
+                       {trace.EventRegionBegin, trace.BackgroundTask, []string{"post-existing region"}},
                }
                r, err := trace.NewReader(bytes.NewReader(tb))
                if err != nil {