Lock-free updates for Histogram

[utpilla]

Changes

• Implement lock-free updates for Histogram
• This does increase the size of MetricPoint struct as we add a new int which would be used for synchronization

If this approach seems good, I can implement it for HistogramSumCount as well.

Stress Test Results

To better understand the perf improvement, we need to look at two scenarios:

1. There is not much contention to update the same MetricPoint. This was tested by running the following in the Run method of stress test:

TestHistogram.Record(
           random.Next(MaxHistogramMeasurement),
           new("DimName1", DimensionValues[random.Next(0, ArraySize)]),
           new("DimName2", DimensionValues[random.Next(0, ArraySize)]),
           new("DimName3", DimensionValues[random.Next(0, ArraySize)]));

3. There is high contention to update the same MetricPoint. This was tested by running the following in the Run method of stress test:

TestHistogram.Record(
           random.Next(MaxHistogramMeasurement),
           new("DimName1", "DimVal1"),
           new("DimName2", "DimVal2"),
           new("DimName3", "DimVal3"));

While there is a perf improvement in both the cases, there is a substantial improvement in the second case where there is high contention to update the same MetricPoint.

For the first scenario, Loops/ second go up from ~20M to ~23M (~15% increase)
For the second scenario, Loops/second go up from ~5.8M to ~7.6M (~31% increase)

Here are the numbers for the 1st and 2nd scenario respectively:

main branch

With this PR

[codecov]

Codecov Report

Merging #2951 (5e9a98d) into main (6981795) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2951      +/-   ##
==========================================
+ Coverage   83.98%   84.02%   +0.04%     
==========================================
  Files         254      254              
  Lines        8942     8946       +4     
==========================================
+ Hits         7510     7517       +7     
+ Misses       1432     1429       -3     

Impacted Files	Coverage Δ
src/OpenTelemetry/Metrics/HistogramBuckets.cs	100.00% <ø> (ø)
...ementation/HttpHandlerMetricsDiagnosticListener.cs	94.11% <100.00%> (ø)
src/OpenTelemetry/Metrics/MetricPoint.cs	86.02% <100.00%> (+0.42%)	⬆️
src/OpenTelemetry/BatchExportProcessor.cs	87.36% <0.00%> (+3.15%)	⬆️

[reyang]

While there is a perf improvement in both the cases, there is a substantial improvement in the second case where there is high contention to update the same MetricPoint.

Great analysis! 👍

[reyang]

I haven't look into the underlying code, wonder if this ++ might throw (e.g. integer overflow case). If that's the case, we might need to make sure we release the spinlock.

[utpilla]

We don't acquire any lock in the first place to be released. SpinWait is used to just smartly apply context switch for the thread.

[reyang]

one Ln335 we set this.histogramBuckets.UsingHistogram = 0, if we throw before this, all other threads would spin I guess?

[reyang]

I think unchecked would solve the problem here.

[cijothomas]

https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/Internal/CircularBuffer.cs#L139-L143 We could explore if we need to have an exit plan like done for CircularBuffer.

[reyang]

LGTM once https://github.com/open-telemetry/opentelemetry-dotnet/pull/2951/files#r815210688 is confirmed.

[CodeBlanch]

Approach LGTM 🚀

[cijothomas]

Please add to changelog and we are good to go.