Optimize histogram reservoir

[dashpole]

This improves the concurrent performance of the histogram reservoir's Offer function by 4x (i.e. 75% reduction).

Accomplish this by locking each measurement, rather than locking around the entire storage. Also, defer extracting the trace context from context.Context until collection time. This improves the performance of Offer, which is on the measure hot path. Exemplars are often overwritten, so deferring the operation until Collect reduces the overall work.

goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/metric/exemplar
cpu: AMD EPYC 7B12
                           │   main.txt   │              hist.txt              │
                           │    sec/op    │   sec/op     vs base               │
FixedSizeReservoirOffer-24    211.4n ± 3%   177.5n ± 3%  -16.04% (p=0.002 n=6)
HistogramReservoirOffer-24   200.85n ± 2%   47.41n ± 2%  -76.40% (p=0.002 n=6)
geomean                       206.1n        91.73n       -55.48%

                           │   main.txt   │              hist.txt              │
                           │     B/op     │    B/op     vs base                │
FixedSizeReservoirOffer-24   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=6) ¹
HistogramReservoirOffer-24   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=6) ¹
geomean                                 ²               +0.00%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                           │   main.txt   │              hist.txt              │
                           │  allocs/op   │ allocs/op   vs base                │
FixedSizeReservoirOffer-24   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=6) ¹
HistogramReservoirOffer-24   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=6) ¹
geomean                                 ²               +0.00%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

I explored using a []atomic.Pointer[measurement], but this had similar performance while being much more complex (needing a sync.Pool to eliminate allocations). The single-threaded performance was also much worse for that solution. See main...dashpole:optimize_histogram_reservoir_old.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.2%. Comparing base (9dea78c) to head (7d0f036).

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #7443     +/-   ##
=======================================
- Coverage   86.2%   86.2%   -0.1%     
=======================================
  Files        295     295             
  Lines      25864   25863      -1     
=======================================
- Hits       22307   22303      -4     
- Misses      3184    3187      +3     
  Partials     373     373             

Files with missing lines	Coverage Δ
sdk/metric/exemplar/fixed_size_reservoir.go	97.6% <100.0%> (ø)
sdk/metric/exemplar/histogram_reservoir.go	92.0% <100.0%> (-1.4%)	⬇️
sdk/metric/exemplar/storage.go	100.0% <100.0%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bboreham]

On further reflection, I fixed the copying issue before running the benchmark, so it is perhaps reasonable that less racy code runs slower.

Would be good if the tests and/or linter detected the issue. I note that NoCopy was removed from atomic.Value here: golang/go#21504.

[dashpole]

I also see slightly worse results, but agree it is definitely better to be correct. I'll work on a test.

[dashpole]

I added a ConcurrentSafe test, and verified that it fails (quite spectacularly) with the previous atomic.Value implementation.

[bboreham]

lgtm

[dashpole]

The concurrent safe test found another race condition around my usage of sync.Pool, which i'm looking into

[dashpole]

The other race had to do with my usage of sync.Pool. After Collect loaded an element, that element could be placed into the sync.Pool by a subsequent store() that replaced the measurement, and then modified by another store() that retrieved it from the sync.Pool. I worked out a way to fix this, but it made the performance around ~45ns. In the end, I decided to just lock around each measurement, since that has the same parallel performance, is much more simple and readable, and has better single-threaded performance.

[bboreham]

Much simpler now.

[bboreham]

drive-by: I think this (and all similar code) should be len not cap.
In the current code they are always the same, but it's a slight jar when reading it to wonder what was intended.