[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 4 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4416 +/- ##
=======================================
Coverage 82.69% 82.69%
=======================================
Files 505 505
Lines 19933 19933
=======================================
Hits 16482 16482
Misses 3451 3451
🚀 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.
| MetricAttributes attrs_copy; | ||
| { | ||
| std::lock_guard<opentelemetry::common::SpinLockMutex> g(entry->lock_); | ||
| std::lock_guard<std::mutex> g(entry->lock_); |
There was a problem hiding this comment.
The above comments still says spinlock while it has chanegd to std::mutex.
3 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 library.For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes