[SDK] Replace SpinLockMutex with std::mutex in the metrics library - #4416
[SDK] Replace SpinLockMutex with std::mutex in the metrics library#4416dbarker wants to merge 9 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4416 +/- ##
==========================================
+ Coverage 83.09% 83.10% +0.01%
==========================================
Files 519 519
Lines 20253 20253
==========================================
+ Hits 16827 16829 +2
+ Misses 3426 3424 -2
🚀 New features to boost your workflow:
|
|
|
||
| private: | ||
| mutable opentelemetry::common::SpinLockMutex lock_; | ||
| mutable std::mutex lock_; |
There was a problem hiding this comment.
I've made the swap to std::mutex here also to be consistent throughout the SDK. This use case for sum aggregation could be a good use for a spinlock but testing showed it was ~10ns difference with the mutex in the uncontested case. This performance delta is not significant compared to the attribute hash map lookup.
There was a problem hiding this comment.
The attribute-map comparison makes sense for the unbound path, but the bound path skips that lookup. It now takes the BoundEntry mutex and then this mutex for every sum update, so the reported 10 ns may matter more there.
Could we capture before-and-after results for the existing bound and unbound counter benchmarks, including a contended case? Since #4317 originally kept this spinlock and requested benchmarks for part 2, I think we should have that evidence before changing this hot path.
There was a problem hiding this comment.
This could increase memory usage significantly. Measured with MSVC x64 but it should similar for Linux.
| type | before (bytes) | after (bytes) |
|---|---|---|
LongSumAggregation |
40 | 112 |
LongLastValueAggregation |
48 | 120 |
LongHistogramAggregation |
136 | 208 |
The driver is that sizeof(std::mutex) is 80 bytes on MSVC and 40 on glibc, while sizeof(std::atomic<bool>) is 1. One aggregation object is allocated per time series, and the default cardinality limit is 2000. That alone is 2000 x 72 = ~144 KB extra per counter instrument. TemporalMetricStorage keeps a per-collector stash of previously reported aggregations, so a configuration with one reader roughly doubles it, reaching near 300 KB per instrument.
There was a problem hiding this comment.
@lalitb @ThomsonTan Thanks for pushing on performance and resource usage.
I've run the spinlock/std::mutex swap through the sync instrument recording benchmark from #4470 and pushed the results to a branch on my fork.
The results are as expected:
- Unbound instruments are largely unaffected by the change (SyncMetricStorage already uses a mutex)
- Bound instruments gain the expected context switch latency (up to 600 ns) under contention.
Looking into the code this contention in the Bound instrument case seems be at the BoundEntry::lock_ and not the lock at the aggregation level. A scenario I did not test was multiple threads recording to the same stream through bound and unbound instruments.
Benchmark Results:
Unbound sync instrument benchmarks (click to view)
| Benchmark | Threads | SpinLockMutex (ns) | std::mutex (ns) | Delta |
|---|---|---|---|---|
| Counter/Drop | 1 | 291 | 287 | -1% |
| Counter/Drop | 2 | 545 | 523 | -4% |
| Counter/Drop | 4 | 1222 | 1224 | +0% |
| Counter/Sum | 1 | 293 | 290 | -1% |
| Counter/Sum | 2 | 546 | 541 | -1% |
| Counter/Sum | 4 | 1372 | 1399 | +2% |
| Histogram/Explicit | 1 | 297 | 300 | +1% |
| Histogram/Explicit | 2 | 746 | 683 | -8% |
| Histogram/Explicit | 4 | 1759 | 1626 | -8% |
| Histogram/Base2Expo | 1 | 300 | 299 | 0% |
| Histogram/Base2Expo | 2 | 588 | 672 | +14% |
| Histogram/Base2Expo | 4 | 1500 | 1607 | +7% |
| Gauge/Drop | 2 | 546 | 665 | +22% |
| Gauge/Drop | 4 | 1200 | 1375 | +15% |
| Gauge/LastValue | 1 | 293 | 304 | +4% |
| Gauge/LastValue | 2 | 564 | 702 | +24% |
| Gauge/LastValue | 4 | 1513 | 1649 | +9% |
Bound sync instrument benchmarks (click to view)
| Benchmark | Threads | SpinLockMutex (ns) | std::mutex (ns) | Delta |
|---|---|---|---|---|
| BoundCounter/Drop | 1 | 5.81 | 11.5 | +98% |
| BoundCounter/Drop | 2 | 34.8 | 68.9 | +98% |
| BoundCounter/Drop | 4 | 123 | 269 | +119% |
| BoundCounter/Sum | 1 | 10.7 | 20.9 | +95% |
| BoundCounter/Sum | 2 | 19.6 | 191 | +875% |
| BoundCounter/Sum | 4 | 73.4 | 417 | +468% |
| BoundHistogram/Drop | 1 | 6.19 | 11.8 | +91% |
| BoundHistogram/Drop | 2 | 32.6 | 70.7 | +117% |
| BoundHistogram/Drop | 4 | 155 | 239 | +54% |
| BoundHistogram/Explicit | 1 | 11.8 | 22.8 | +93% |
| BoundHistogram/Explicit | 2 | 23.6 | 409 | +1634% |
| BoundHistogram/Explicit | 4 | 196 | 546 | +179% |
| BoundHistogram/Base2Expo | 1 | 19.3 | 27.7 | +44% |
| BoundHistogram/Base2Expo | 2 | 32.8 | 485 | +1379% |
| BoundHistogram/Base2Expo | 4 | 67.1 | 674 | +905% |
Why swap to std::mutex regardless of the impact on Bound instrument benchmarks?
The SpinLockMutex implementation is problematic for several reasons:
- It is not complete for all platforms with fast_yield still a TODO.
- The sleep behavior is not standard for a spinlock and varies significantly across platforms (on Windows this may be 15+ ms).
- We don't have the systems or tools available to benchmark performance (p95,p99) of
SpinLockMutexin the various platforms and deployments of the SDK (on a single core, in a virtual machine, ARM devices, ...) so the impact of the bespokeSpinLockMutexis unknown. - std::mutex is optimized on many platforms and will not break down in pathological ways as the
SpinLockMutexwill under contention or with unexpected extension of critical sections.
Proposed next steps:
I propose we proceed to swap all storage level SpinLockMutex's to std::mutex and only keep SpinLockMutex at the aggregation level to save memory allocation (@ThomsonTan's concern) if they are truly never contended. If never contended then we may consider just removing them from the aggregation level.
Questions:
- Is the aggregation lock always uncontended and still needed?
- Is simultaneous recording to a metrics stream through Bound and Unbound instruments tested and supported?
- Do you have any feedback on the proposed next steps?
There was a problem hiding this comment.
As discussed in SIG meeting, I propose to rollback changes to *_aggregation.h.
Locks for aggregate methods to investigate separately.
4 participants
Fixes #4317
Part 2 of #4317 to replace SpinLockMutex with std::mutex.
This PR replaces all use of the SpinLockMutex in the metrics sdk. The goal is to improve synchronization consistency across platforms and deployments by relying on the standard mutex.
Changes
SpinLockMutexwithstd::mutexin the metrics storage classes.For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes