From 0ed4466403a5842ce84c3d24cdabcdccee20ddf3 Mon Sep 17 00:00:00 2001 From: David Hartunian Date: Wed, 11 Dec 2024 14:58:39 -0500 Subject: [PATCH] tracing: pass `spanOptions` by reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reduces a bunch of struct copying. For now, I'm considering the `none/parallel` benchmark diffs to be spurious. ``` ❯ benchdiff ./pkg/util/tracing -b -r 'BenchmarkTracer_StartSpanCtx' -d 100000x -c 10 test binaries already exist for 2b3e4f4: Merge #136225 #137136 #137216 #137275 #137281 checking out '9899e2d' building benchmark binaries for 9899e2d: tracing: pass `spanOptions` by reference [bazel=true] 1/1 \ pkg=1/1 iter=10/10 cockroachdb/cockroach/pkg/util/tracing \ name old time/op new time/op delta Tracer_StartSpanCtx/opts=none/parallel=false-10 50.0ns ± 6% 45.5ns ± 4% -8.93% (p=0.000 n=10+9) Tracer_StartSpanCtx/opts=real,autoparent/parallel=false-10 321ns ± 4% 301ns ± 1% -6.36% (p=0.000 n=10+8) Tracer_StartSpanCtx/opts=real,manualparent/parallel=false-10 343ns ± 4% 324ns ± 2% -5.60% (p=0.000 n=9+9) Tracer_StartSpanCtx/opts=real,manualparent,withEventListener/parallel=false-10 358ns ± 2% 341ns ± 2% -4.71% (p=0.000 n=10+9) Tracer_StartSpanCtx/opts=real,autoparent,withEventListener/parallel=false-10 472ns ± 2% 457ns ± 2% -3.27% (p=0.000 n=8+9) Tracer_StartSpanCtx/opts=real/parallel=false-10 292ns ± 2% 282ns ± 2% -3.15% (p=0.000 n=8+9) Tracer_StartSpanCtx/opts=real,manualparent,withEventListener/parallel=true-10 663ns ± 2% 643ns ± 4% -3.11% (p=0.002 n=9+9) Tracer_StartSpanCtx/opts=real,logtag/parallel=false-10 309ns ± 2% 300ns ± 2% -3.02% (p=0.000 n=9+9) Tracer_StartSpanCtx/opts=real,autoparent/parallel=true-10 621ns ± 3% 608ns ± 4% -2.17% (p=0.039 n=7+9) Tracer_StartSpanCtx/opts=real,regexp/parallel=true-10 826ns ± 2% 810ns ± 2% -1.95% (p=0.008 n=10+9) Tracer_StartSpanCtx/opts=real/parallel=true-10 556ns ± 5% 561ns ± 3% ~ (p=0.661 n=10+9) Tracer_StartSpanCtx/opts=real,logtag/parallel=true-10 580ns ± 2% 574ns ± 2% ~ (p=0.077 n=9+9) Tracer_StartSpanCtx/opts=real,manualparent/parallel=true-10 635ns ± 3% 625ns ± 3% ~ (p=0.074 n=8+9) Tracer_StartSpanCtx/opts=real,autoparent,withEventListener/parallel=true-10 1.04µs ± 6% 1.03µs ± 5% ~ (p=0.735 n=10+9) Tracer_StartSpanCtx/opts=real,regexp/parallel=false-10 745ns ± 1% 739ns ± 1% ~ (p=0.078 n=8+8) Tracer_StartSpanCtx/opts=none/parallel=true-10 15.3ns ±19% 22.1ns ±27% +44.39% (p=0.000 n=9+9) name old alloc/op new alloc/op delta Tracer_StartSpanCtx/opts=none/parallel=false-10 48.0B ± 0% 48.0B ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=none/parallel=true-10 48.0B ± 0% 48.0B ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real/parallel=false-10 48.0B ± 0% 48.0B ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real/parallel=true-10 48.0B ± 0% 48.0B ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,logtag/parallel=false-10 48.0B ± 0% 48.0B ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,logtag/parallel=true-10 48.0B ± 0% 48.0B ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,autoparent/parallel=false-10 48.0B ± 0% 48.0B ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,autoparent/parallel=true-10 48.0B ± 0% 48.0B ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,manualparent/parallel=false-10 48.0B ± 0% 48.0B ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,manualparent/parallel=true-10 48.0B ± 0% 48.0B ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,autoparent,withEventListener/parallel=false-10 96.0B ± 0% 96.0B ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,autoparent,withEventListener/parallel=true-10 96.0B ± 0% 96.0B ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,manualparent,withEventListener/parallel=false-10 48.0B ± 0% 48.0B ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,manualparent,withEventListener/parallel=true-10 48.0B ± 0% 48.0B ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,regexp/parallel=false-10 48.0B ± 0% 48.0B ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,regexp/parallel=true-10 51.0B ± 0% 51.0B ± 0% ~ (all equal) name old allocs/op new allocs/op delta Tracer_StartSpanCtx/opts=none/parallel=false-10 1.00 ± 0% 1.00 ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=none/parallel=true-10 1.00 ± 0% 1.00 ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real/parallel=false-10 1.00 ± 0% 1.00 ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real/parallel=true-10 1.00 ± 0% 1.00 ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,logtag/parallel=false-10 1.00 ± 0% 1.00 ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,logtag/parallel=true-10 1.00 ± 0% 1.00 ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,autoparent/parallel=false-10 1.00 ± 0% 1.00 ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,autoparent/parallel=true-10 1.00 ± 0% 1.00 ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,manualparent/parallel=false-10 1.00 ± 0% 1.00 ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,manualparent/parallel=true-10 1.00 ± 0% 1.00 ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,autoparent,withEventListener/parallel=false-10 2.00 ± 0% 2.00 ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,autoparent,withEventListener/parallel=true-10 2.00 ± 0% 2.00 ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,manualparent,withEventListener/parallel=false-10 1.00 ± 0% 1.00 ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,manualparent,withEventListener/parallel=true-10 1.00 ± 0% 1.00 ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,regexp/parallel=false-10 1.00 ± 0% 1.00 ± 0% ~ (all equal) Tracer_StartSpanCtx/opts=real,regexp/parallel=true-10 1.00 ± 0% 1.00 ± 0% ~ (all equal) ``` --- pkg/util/tracing/tracer.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/pkg/util/tracing/tracer.go b/pkg/util/tracing/tracer.go index 4e112d5fc85f..6ae4a0840e90 100644 --- a/pkg/util/tracing/tracer.go +++ b/pkg/util/tracing/tracer.go @@ -1078,7 +1078,7 @@ func (t *Tracer) StartSpanCtx( opts = os[i].apply(opts) } - return t.startSpanFast(ctx, operationName, opts) + return t.startSpanFast(ctx, operationName, &opts) } // AlwaysTrace returns true if operations should be traced regardless of the @@ -1098,8 +1098,12 @@ func (t *Tracer) forceOpNameVerbose(opName string) bool { return false } +// startSpanFast implements a fast path for the common case of tracing +// being disabled on the current span and its parent. We make only the +// checks necessary to ensure that recording is disabled and wrap the +// context in a `noopSpan`. func (t *Tracer) startSpanFast( - ctx context.Context, opName string, opts spanOptions, + ctx context.Context, opName string, opts *spanOptions, ) (context.Context, *Span) { if opts.RefType != childOfRef && opts.RefType != followsFromRef { panic(errors.AssertionFailedf("unexpected RefType %v", opts.RefType)) @@ -1136,7 +1140,7 @@ func (t *Tracer) startSpanFast( // the latter case, ctx == noCtx and the returned Context is the supplied one; // otherwise the returned Context embeds the returned Span. func (t *Tracer) startSpanGeneric( - ctx context.Context, opName string, opts spanOptions, + ctx context.Context, opName string, opts *spanOptions, ) (context.Context, *Span) { if opts.RefType != childOfRef && opts.RefType != followsFromRef { panic(errors.AssertionFailedf("unexpected RefType %v", opts.RefType))