open-telemetry / opentelemetry-go-contrib

Collection of extensions for OpenTelemetry-Go.
https://opentelemetry.io/
Apache License 2.0
1.15k stars 544 forks source link

Revert #5842 #6128

Open MrAlias opened 1 week ago

MrAlias commented 1 week ago

May I ask why the change, approved in https://github.com/open-telemetry/opentelemetry-go-contrib/pull/5664, was reverted? Vararg slice was allocated once and used 2/multiple times. Now it is allocated on each Add() call. There is/was even a comment, explaining the "why".

See https://go.dev/ref/spec#Passing_arguments_to_..._parameters

See https://fs.blog/chestertons-fence/

var sink []string

func abc(vals ...string) {
    sink = vals
}

func BenchmarkFuncCall(b *testing.B) {
    b.ReportAllocs()
    b.ResetTimer()
    arg := []string{"a", "b", "c"}
    for i := 0; i < b.N; i++ {
        abc(arg...)
    }
}

gives me

BenchmarkFuncCall-10        1000000000           0.9481 ns/op          0 B/op          0 allocs/op
var sink []string

func abc(vals ...string) {
    sink = vals
}

func BenchmarkFuncCall(b *testing.B) {
    b.ReportAllocs()
    b.ResetTimer()
    for i := 0; i < b.N; i++ {
        abc("a", "b", "c")
    }
}

gives me

BenchmarkFuncCall-10        42442630            26.69 ns/op       48 B/op          1 allocs/op

Put the fence back, please 😁

Originally posted by @ash2k in https://github.com/open-telemetry/opentelemetry-go-contrib/issues/5842#issuecomment-2339603026

XSAM commented 1 week ago

Should we use a more explainable fence for the comment? Something like Allocate slice once to avoid allocation for the variadic argument.

MrAlias commented 1 week ago

SGTM