opentelemetry-go icon indicating copy to clipboard operation
opentelemetry-go copied to clipboard

trace: use weak in TracerProvider for storing tracers

Open yumosx opened this issue 7 months ago • 5 comments

#6351

unique name

goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/otel/sdk/trace
cpu: Apple M3
                       │ BenchmarkTracerWithUniqueName_output.txt │     afterUniqueName_output.txt      │
                       │                  sec/op                  │   sec/op     vs base                │
TracerWithUniqueName-8                                389.9n ± 9%   918.1n ± 8%  +135.44% (p=0.002 n=6)

                       │ BenchmarkTracerWithUniqueName_output.txt │ afterUniqueName_output.txt  │
                       │                   B/op                   │    B/op     vs base         │
TracerWithUniqueName-8                                543.5 ± 15%   550.0 ± 0%  ~ (p=1.000 n=6)

                       │ BenchmarkTracerWithUniqueName_output.txt │     afterUniqueName_output.txt      │
                       │                allocs/op                 │  allocs/op   vs base                │
TracerWithUniqueName-8                                 4.000 ± 0%   11.000 ± 0%  +175.00% (p=0.002 n=6)

same name:

goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/otel/sdk/trace
cpu: Apple M3
                     │ BenchmarkTracerWithSameName_output.txt │      afterSameName_output.txt       │
                     │                 sec/op                 │   sec/op     vs base                │
TracerWithSameName-8                              20.84n ± 1%   51.05n ± 1%  +144.88% (p=0.002 n=6)

                     │ BenchmarkTracerWithSameName_output.txt │   afterSameName_output.txt    │
                     │                  B/op                  │    B/op     vs base           │
TracerWithSameName-8                               0.000 ± 0%   0.000 ± 0%  ~ (p=1.000 n=6) ¹
¹ all samples are equal

                     │ BenchmarkTracerWithSameName_output.txt │   afterSameName_output.txt    │
                     │               allocs/op                │ allocs/op   vs base           │
TracerWithSameName-8                               0.000 ± 0%   0.000 ± 0%  ~ (p=1.000 n=6) ¹
¹ all samples are equal

yumosx avatar Apr 13 '25 10:04 yumosx

HI @pellared, I used weak to store the tracer (following the official Go blog's approach), but after performance testing, the results weren't good - performance actually decreased.

Am I using the wrong approach here?

yumosx avatar Apr 13 '25 11:04 yumosx

The linked issue in the description seems unrelated to this PR.

Note that we won't be able to merge this until 1.25 is released (which is when we will drop support for 1.23).

dmathieu avatar Apr 15 '25 09:04 dmathieu

The linked issue in the description seems unrelated to this PR.

Sorry, my bad, updated.

Note that we won't be able to merge this until 1.25 is released (which is when we will drop support for 1.23).

I know it, this pr is submitted as a PoC for early review.

as @pellared say

I hope we both learned something useful.

yumosx avatar Apr 15 '25 09:04 yumosx

Hi, currently I have no time to review this PR, but I just want to call out that use of weak would not "improve" the performance (on the hot-path). It will only give the garbage collector the possibility to remove tracers that are not referenced by the user. So the total used memory should be lower (after garbage collection) when tracers are "used and forgotten" by the user (caller).

pellared avatar Apr 17 '25 08:04 pellared

use of weak would not "improve" the performance (on the hot-path). It will only give the garbage collector the possibility to remove tracers that are not referenced by the user.

Yes, this is right, The initial Benchmark test show map[instrument.Scope]*tracer performs better than weak.Pointer[tracer].

So the total used memory should be lower (after garbage collection) when tracers are "used and forgotten" by the user (caller).

the second test, forcing runtime.GC() shows the weak+cleanup version runs faster than without it.

I think this improvement comes from weak+cleanup lowering GC pressure.

finally, thank you for the insightful reply.

yumosx avatar Apr 17 '25 10:04 yumosx