Use Volatile instead of Interlocked where appropriate #6051
Conversation
github-actions
bot
added
pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol
| @@ -394,7 +394,7 @@ internal void Update(long number) | |||
| case AggregationType.LongSumIncomingCumulative: | |||
| case AggregationType.LongGauge: | |||
| { | |||
| Interlocked.Exchange(ref this.runningValue.AsLong, number); | |||
| Volatile.Write(ref this.runningValue.AsLong, number); | |||
There was a problem hiding this comment.
curious, if this shows improvement in the metric stress tests?
| @@ -135,7 +135,7 @@ internal void Update<T>(in ExemplarMeasurement<T> measurement) | |||
| this.StoreRawTags(measurement.Tags); | |||
| } | |||
|
|
|||
| Interlocked.Exchange(ref this.isCriticalSectionOccupied, 0); | |||
| Volatile.Write(ref this.isCriticalSectionOccupied, 0); | |||
There was a problem hiding this comment.
We need to consider the memory ordering guarantees of Volatile.Write. With Interlocked methods, the read/writes would not be moved before or after a given Interlocked method.
With volatile writes, read/writes that happen after a given Volatile.Write method can be moved before that Volatile.Write method. We need to evaluate if that affects the correctness of our code. There are some write operations that we do after releasing the locks (for exemplar and MetricPoint updates):
- Call
OnCollectedfor Exemplars which resets the internal measurement state - Update
MetricStatustoCollectPendingwhen updatingMetricPoints
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
github-actions
bot
added
the
Stale
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6051 +/- ##
==========================================
- Coverage 86.39% 86.39% -0.01%
==========================================
Files 257 257
Lines 11690 11668 -22
==========================================
- Hits 10100 10080 -20
+ Misses 1590 1588 -2
|
Volatile reads/writes are atomic and have acquire/release semantics, but are for the most part as fast as regular reads/writes. Any interlocked operation is at least 30-40 CPU cycles and needs exclusive cache line ownership, which is especially bad for reads.
Split off from #6048.