open-telemetry / opentelemetry-go

OpenTelemetry Go API and SDK
https://opentelemetry.io/
Apache License 2.0
5.01k stars 1.02k forks source link

Check for instantiation of generic for type inference. #5511

Open MadVikingGod opened 1 month ago

MadVikingGod commented 1 month ago
          The majority of this change is due to these lines. Local testing, removing these lines and leaving it as generic, results in about -300ns, -304B, and -4 Allocs. 

          We (the sig) should check if this pattern is used elsewhere, as it's probably causing performance issues.

_Originally posted by @MadVikingGod in https://github.com/open-telemetry/opentelemetry-go/pull/5497#discussion_r1638456410_

This PR highlighted that this pattern of creating a Generic variable to make typer inferences is very costly. We should search to see if this pattern is used elsewhere and see if removing it gives us similar performance improvements.

dmathieu commented 4 weeks ago

From a quick grep, I'm finding only one non-test occurence.

open-telemetry/opentelemetry-go›  git:(main) grep -rn 'var .* T' .
./trace/config.go:38:   var config TracerConfig
./trace/tracestate.go:175:var _ json.Marshaler = TraceState{}
./trace/noop.go:26:var _ TracerProvider = noopTracerProvider{}
./trace/noop.go:36:var _ Tracer = noopTracer{}
./trace/tracestate_test.go:279:var maxMembers = func() TraceState {
./trace/tracestate_test.go:412:var insertTS = TraceState{list: []member{
./exporters/zipkin/internal/internaltest/errors.go:11:var _ error = TestError("")
./internal/internaltest/errors.go:11:var _ error = TestError("")
./internal/shared/internaltest/errors.go.tmpl:11:var _ error = TestError("")
./sdk/trace/evictedqueue.go:23: var tVal T
./sdk/internal/internaltest/errors.go:11:var _ error = TestError("")
./sdk/internal/x/x_test.go:50:  var zero T
./sdk/metric/internal/x/x_test.go:60:   var zero T
./sdk/metric/view.go:112:       var zero T
./propagation/propagation.go:37:var _ TextMapCarrier = MapCarrier{}
./propagation/baggage.go:20:var _ TextMapPropagator = Baggage{}

https://github.com/open-telemetry/opentelemetry-go/blob/499785e13488854518260065716ec1094bf69f12/sdk/metric/view.go#L110-L117

We could remove the generics in this method, by passing a third argument into the method which would be the zero value. Creating views shouldn't be in the hot path though, as it happens once at boot time.