Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for instantiation of generic for type inference. #5511

Open
MadVikingGod opened this issue Jun 13, 2024 · 1 comment
Open

Check for instantiation of generic for type inference. #5511

MadVikingGod opened this issue Jun 13, 2024 · 1 comment

Comments

@MadVikingGod
Copy link
Contributor

MadVikingGod commented Jun 13, 2024

          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 #5497 (comment)

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 dmathieu changed the title Check for instinaciation of generic for type inference. Check for instanciation of generic for type inference. Jun 17, 2024
@dmathieu
Copy link
Member

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{}

// nonZero returns v if it is non-zero-valued, otherwise alt.
func nonZero[T comparable](v, alt T) T {
var zero T
if v != zero {
return v
}
return alt
}

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.

@MrAlias MrAlias changed the title Check for instanciation of generic for type inference. Check for instantiation of generic for type inference. Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants