Remove Interlocked -> Volatile changes

open-telemetry · Jan 8, 2025 · 3593cee · 3593cee
1 parent 4212345
commit 3593cee
Show file tree

Hide file tree

Showing 5 changed files with 36 additions and 19 deletions.
diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/PersistentStorage/DirectorySizeTracker.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/PersistentStorage/DirectorySizeTracker.cs
@@ -36,14 +36,14 @@ public DirectorySizeTracker(long maxSizeInBytes, string path)
     /// <returns>True if space is available else false.</returns>
     public bool IsSpaceAvailable(out long currentSizeInBytes)
     {
-        currentSizeInBytes = Volatile.Read(ref this.directoryCurrentSizeInBytes);
+        currentSizeInBytes = Interlocked.Read(ref this.directoryCurrentSizeInBytes);
         return currentSizeInBytes < this.maxSizeInBytes;
     }
 
     public void RecountCurrentSize()
     {
         var size = CalculateFolderSize(this.path);
-        Volatile.Write(ref this.directoryCurrentSizeInBytes, size);
+        Interlocked.Exchange(ref this.directoryCurrentSizeInBytes, size);
     }
 
     internal static long CalculateFolderSize(string path)

diff --git a/src/OpenTelemetry/Internal/InterlockedHelper.cs b/src/OpenTelemetry/Internal/InterlockedHelper.cs
@@ -10,9 +10,30 @@ internal static class InterlockedHelper
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
     public static void Add(ref double location, double value)
     {
+        // Note: Not calling InterlockedHelper.Read here on purpose because it
+        // is too expensive for fast/happy-path. If the first attempt fails
+        // we'll end up in an Interlocked.CompareExchange loop anyway.
         double currentValue = Volatile.Read(ref location);
+
+        var returnedValue = Interlocked.CompareExchange(ref location, currentValue + value, currentValue);
+        if (returnedValue != currentValue)
+        {
+            AddRare(ref location, value, returnedValue);
+        }
+    }
+
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    public static double Read(ref double location)
+        => Interlocked.CompareExchange(ref location, double.NaN, double.NaN);
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static void AddRare(ref double location, double value, double currentValue)
+    {
+        var sw = default(SpinWait);
         while (true)
         {
+            sw.SpinOnce();
+
             var returnedValue = Interlocked.CompareExchange(ref location, currentValue + value, currentValue);
             if (returnedValue == currentValue)
             {
@@ -22,8 +43,4 @@ public static void Add(ref double location, double value)
             currentValue = returnedValue;
         }
     }
-
-    [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    public static double Read(ref double location)
-        => Interlocked.CompareExchange(ref location, 0, 0);
 }
diff --git a/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs b/src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs
@@ -135,7 +135,7 @@ internal void Update<T>(in ExemplarMeasurement<T> measurement)
             this.StoreRawTags(measurement.Tags);
         }
 
-        Volatile.Write(ref this.isCriticalSectionOccupied, 0);
+        Interlocked.Exchange(ref this.isCriticalSectionOccupied, 0);
     }
 
     internal void Reset()
@@ -168,7 +168,7 @@ internal void Collect(ref Exemplar destination, bool reset)
             destination.Reset();
         }
 
-        Volatile.Write(ref this.isCriticalSectionOccupied, 0);
+        Interlocked.Exchange(ref this.isCriticalSectionOccupied, 0);
     }
 
     internal readonly void Copy(ref Exemplar destination)

diff --git a/src/OpenTelemetry/Metrics/MetricPoint/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint/MetricPoint.cs
@@ -394,7 +394,7 @@ internal void Update(long number)
             case AggregationType.LongSumIncomingCumulative:
             case AggregationType.LongGauge:
                 {
-                    Volatile.Write(ref this.runningValue.AsLong, number);
+                    Interlocked.Exchange(ref this.runningValue.AsLong, number);
                     break;
                 }
 
@@ -451,7 +451,7 @@ internal void UpdateWithExemplar(long number, ReadOnlySpan<KeyValuePair<string,
             case AggregationType.LongSumIncomingCumulative:
             case AggregationType.LongGauge:
                 {
-                    Volatile.Write(ref this.runningValue.AsLong, number);
+                    Interlocked.Exchange(ref this.runningValue.AsLong, number);
                     break;
                 }
 
@@ -510,7 +510,7 @@ internal void Update(double number)
             case AggregationType.DoubleSumIncomingCumulative:
             case AggregationType.DoubleGauge:
                 {
-                    Volatile.Write(ref this.runningValue.AsDouble, number);
+                    Interlocked.Exchange(ref this.runningValue.AsDouble, number);
                     break;
                 }
 
@@ -567,7 +567,7 @@ internal void UpdateWithExemplar(double number, ReadOnlySpan<KeyValuePair<string
             case AggregationType.DoubleSumIncomingCumulative:
             case AggregationType.DoubleGauge:
                 {
-                    Volatile.Write(ref this.runningValue.AsDouble, number);
+                    Interlocked.Exchange(ref this.runningValue.AsDouble, number);
                     break;
                 }
 
@@ -622,7 +622,7 @@ internal void TakeSnapshot(bool outputDelta)
                 {
                     if (outputDelta)
                     {
-                        long initValue = Volatile.Read(ref this.runningValue.AsLong);
+                        long initValue = Interlocked.Read(ref this.runningValue.AsLong);
                         this.snapshotValue.AsLong = initValue - this.deltaLastValue.AsLong;
                         this.deltaLastValue.AsLong = initValue;
                         this.MetricPointStatus = MetricPointStatus.NoCollectPending;
@@ -636,7 +636,7 @@ internal void TakeSnapshot(bool outputDelta)
                     }
                     else
                     {
-                        this.snapshotValue.AsLong = Volatile.Read(ref this.runningValue.AsLong);
+                        this.snapshotValue.AsLong = Interlocked.Read(ref this.runningValue.AsLong);
                     }
 
                     break;
@@ -647,7 +647,7 @@ internal void TakeSnapshot(bool outputDelta)
                 {
                     if (outputDelta)
                     {
-                        double initValue = Volatile.Read(ref this.runningValue.AsDouble);
+                        double initValue = InterlockedHelper.Read(ref this.runningValue.AsDouble);
                         this.snapshotValue.AsDouble = initValue - this.deltaLastValue.AsDouble;
                         this.deltaLastValue.AsDouble = initValue;
                         this.MetricPointStatus = MetricPointStatus.NoCollectPending;
@@ -661,15 +661,15 @@ internal void TakeSnapshot(bool outputDelta)
                     }
                     else
                     {
-                        this.snapshotValue.AsDouble = Volatile.Read(ref this.runningValue.AsDouble);
+                        this.snapshotValue.AsDouble = InterlockedHelper.Read(ref this.runningValue.AsDouble);
                     }
 
                     break;
                 }
 
             case AggregationType.LongGauge:
                 {
-                    this.snapshotValue.AsLong = Volatile.Read(ref this.runningValue.AsLong);
+                    this.snapshotValue.AsLong = Interlocked.Read(ref this.runningValue.AsLong);
                     this.MetricPointStatus = MetricPointStatus.NoCollectPending;
 
                     // Check again if value got updated, if yes reset status.
@@ -684,7 +684,7 @@ internal void TakeSnapshot(bool outputDelta)
 
             case AggregationType.DoubleGauge:
                 {
-                    this.snapshotValue.AsDouble = Volatile.Read(ref this.runningValue.AsDouble);
+                    this.snapshotValue.AsDouble = InterlockedHelper.Read(ref this.runningValue.AsDouble);
                     this.MetricPointStatus = MetricPointStatus.NoCollectPending;
 
                     // Check again if value got updated, if yes reset status.

diff --git a/src/OpenTelemetry/Metrics/MetricPoint/MetricPointOptionalComponents.cs b/src/OpenTelemetry/Metrics/MetricPoint/MetricPointOptionalComponents.cs
@@ -48,7 +48,7 @@ public void AcquireLock()
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
     public void ReleaseLock()
     {
-        Volatile.Write(ref this.isCriticalSectionOccupied, 0);
+        Interlocked.Exchange(ref this.isCriticalSectionOccupied, 0);
     }
 
     // Note: This method is marked as NoInlining because the whole point of it