From 0afe71c4cdb43747353afe3b13760cdfb0393315 Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Tue, 10 Sep 2024 11:33:27 +0200 Subject: [PATCH 1/3] stop: annotate tasks with `trace.Region` From the docs[^1]: > A region is for logging a time interval during a goroutine's > execution. By definition, a region starts and ends in the same > goroutine. Regions can be nested to represent subintervals. `StartRegion` is a no-op when no execution trace is ongoing[^2]. It does make one small allocation otherwise, which is a little silly since a better API could have avoided it. The result of this change is that execution traces are easier to inspect visually, as many goroutines in the system will now state their purpose (the task name) as opposed to looking like yet another opaque "(*Stopper).RunAsyncTaskEx.func2". Sadly, goroutines that tagged their region before the execution trace begins will not reflect their region. So, for example, a `(*raftSchedulerShard).worker`, despite running inside of a task, will not have a region attached to it (since the `StartRegion` call happened at server start, before the execution trace began). [^1]: https://pkg.go.dev/runtime/trace#hdr-User_annotation [^2]: https://cs.opensource.google/go/go/+/refs/tags/go1.23.1:src/runtime/trace/annotation.go;l=144-159 Epic: none Release note: None --- pkg/util/stop/stopper.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/pkg/util/stop/stopper.go b/pkg/util/stop/stopper.go index c9ab5d45a94b..083a1a90011b 100644 --- a/pkg/util/stop/stopper.go +++ b/pkg/util/stop/stopper.go @@ -15,6 +15,7 @@ import ( "fmt" "net/http" "runtime/debug" + "runtime/trace" "sync/atomic" "testing" "time" @@ -315,11 +316,16 @@ func (s *Stopper) RunTask(ctx context.Context, taskName string, f func(context.C // Call f. defer s.recover(ctx) defer s.runPostlude() + defer s.startRegion(ctx, taskName).End() f(ctx) return nil } +func (s *Stopper) startRegion(ctx context.Context, taskName string) *trace.Region { + return trace.StartRegion(ctx, taskName) +} + // RunTaskWithErr is like RunTask(), but takes in a callback that can return an // error. The error is returned to the caller. func (s *Stopper) RunTaskWithErr( @@ -332,6 +338,7 @@ func (s *Stopper) RunTaskWithErr( // Call f. defer s.recover(ctx) defer s.runPostlude() + defer s.startRegion(ctx, taskName).End() return f(ctx) } @@ -472,8 +479,9 @@ func (s *Stopper) RunAsyncTaskEx(ctx context.Context, opt TaskOpts, f func(conte // Call f on another goroutine. taskStarted = true // Another goroutine now takes ownership of the alloc, if any. - go func() { + go func(taskName string) { defer s.runPostlude() + defer s.startRegion(ctx, taskName).End() defer sp.Finish() defer s.recover(ctx) if alloc != nil { @@ -482,7 +490,7 @@ func (s *Stopper) RunAsyncTaskEx(ctx context.Context, opt TaskOpts, f func(conte sp.UpdateGoroutineIDToCurrent() f(ctx) - }() + }(opt.TaskName) return nil } From 3bf34dc3a192d7efeee8aa97e46bf73f817b2b9b Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Thu, 12 Sep 2024 09:05:53 +0200 Subject: [PATCH 2/3] stop: elide trace.Region when no exec trace active `StartRegion` does one allocation, might as well save it in the common case of execution tracing not being active. Epic: none Release note: None --- pkg/util/stop/stopper.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/pkg/util/stop/stopper.go b/pkg/util/stop/stopper.go index 083a1a90011b..6deeba515a61 100644 --- a/pkg/util/stop/stopper.go +++ b/pkg/util/stop/stopper.go @@ -322,7 +322,18 @@ func (s *Stopper) RunTask(ctx context.Context, taskName string, f func(context.C return nil } -func (s *Stopper) startRegion(ctx context.Context, taskName string) *trace.Region { +type region interface { + End() +} + +type noopRegion struct{} + +func (n noopRegion) End() {} + +func (s *Stopper) startRegion(ctx context.Context, taskName string) region { + if !trace.IsEnabled() { + return noopRegion{} + } return trace.StartRegion(ctx, taskName) } From 88f9f70b7d57faec4539cc5126debc02ffc4594f Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Thu, 12 Sep 2024 14:28:40 -0400 Subject: [PATCH 3/3] opt: remove unnecessary pointer indirection This commit removes unnecessary pointer indirection in the join order builder. Release note: None --- pkg/sql/opt/xform/join_order_builder.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/sql/opt/xform/join_order_builder.go b/pkg/sql/opt/xform/join_order_builder.go index 20c6e88075d5..fc3b022a05f5 100644 --- a/pkg/sql/opt/xform/join_order_builder.go +++ b/pkg/sql/opt/xform/join_order_builder.go @@ -709,13 +709,13 @@ func (jb *JoinOrderBuilder) addJoins(s1, s2 vertexSet) { func (jb *JoinOrderBuilder) makeInnerEdge(op *operator, filters memo.FiltersExpr) { if len(filters) == 0 { // This is a cross join. Create a single edge for the empty FiltersExpr. - jb.edges = append(jb.edges, *jb.makeEdge(op, filters)) + jb.edges = append(jb.edges, jb.makeEdge(op, filters)) jb.innerEdges.Add(len(jb.edges) - 1) return } for i := range filters { // Create an edge for each conjunct. - jb.edges = append(jb.edges, *jb.makeEdge(op, filters[i:i+1])) + jb.edges = append(jb.edges, jb.makeEdge(op, filters[i:i+1])) jb.innerEdges.Add(len(jb.edges) - 1) } } @@ -724,7 +724,7 @@ func (jb *JoinOrderBuilder) makeInnerEdge(op *operator, filters memo.FiltersExpr // join. For any given non-inner join, exactly one edge is constructed. func (jb *JoinOrderBuilder) makeNonInnerEdge(op *operator, filters memo.FiltersExpr) { // Always create a single edge from a non-inner join. - jb.edges = append(jb.edges, *jb.makeEdge(op, filters)) + jb.edges = append(jb.edges, jb.makeEdge(op, filters)) jb.nonInnerEdges.Add(len(jb.edges) - 1) } @@ -776,13 +776,13 @@ func (jb *JoinOrderBuilder) makeTransitiveEdge(col1, col2 opt.ColumnID) { filters := memo.FiltersExpr{jb.f.ConstructFiltersItem(condition)} // Add the edge to the join graph. - jb.edges = append(jb.edges, *jb.makeEdge(op, filters)) + jb.edges = append(jb.edges, jb.makeEdge(op, filters)) jb.innerEdges.Add(len(jb.edges) - 1) } // makeEdge returns a new edge given an operator and set of filters. -func (jb *JoinOrderBuilder) makeEdge(op *operator, filters memo.FiltersExpr) (e *edge) { - e = &edge{op: op, filters: filters} +func (jb *JoinOrderBuilder) makeEdge(op *operator, filters memo.FiltersExpr) (e edge) { + e = edge{op: op, filters: filters} e.calcNullRejectedRels(jb) e.calcSES(jb) e.calcTES(jb.edges)