HDDS-9377. Replace MutableStat with lock-free ConcurrentMutableStat in OMLockMetrics and PerformanceMetrics - #11085
HDDS-9377. Replace MutableStat with lock-free ConcurrentMutableStat in OMLockMetrics and PerformanceMetrics#11085yandrey321 wants to merge 4 commits into
Conversation
sumitagrawl
left a comment
sumitagrawl
left a comment
There was a problem hiding this comment.
@yandrey321 Over Looks good, the value may have slight inconsistent over the boundary update. But seems ok for the metrics in high concurrent sceanrio.
Do this is applicable for only given metrics or need update other places.
Additionally, this has some additional memory usages per metrics.
|
|
||
| public ConcurrentMutableStat(String name, String description, | ||
| String sampleName, String valueName, boolean extended) { | ||
| super(name, description, sampleName, valueName, extended); |
There was a problem hiding this comment.
the implementation extended=true may not provide correct value for stdev_time, as this purpose. May be extended can be always false for this class.
There was a problem hiding this comment.
Good catch, stdev_time would be underestimated with the current implementation.
This PR addresses the spots where we observed lock contention issues in MutableStat. I'll open an additional JIRA for updating remaining code that use MutableStat.
Yes, its a trade off for fixing lock contention. |
| "Time (in milliseconds) spent waiting for acquiring the read lock", | ||
| "Ops", "Time", true); | ||
| readLockHeldTimeMsStat = registry.newStat("ReadLockHeldTime", | ||
| "Ops", "Time", false); |
There was a problem hiding this comment.
turning the extended field from true to false means extended stats (stdev, min/max etc.) are not created by default. If ConcurrentMutableStat is not expected to support extended stats, then let's remove this parameter.
| String sampleName, String valueName, boolean extended) { | ||
| super(name, description, sampleName, valueName, extended); |
There was a problem hiding this comment.
| String sampleName, String valueName, boolean extended) { | |
| super(name, description, sampleName, valueName, extended); | |
| String sampleName, String valueName) { | |
| super(name, description, sampleName, valueName, false); |
jojochuang
left a comment
jojochuang
left a comment
There was a problem hiding this comment.
The suggestion I made above can be addressed in a followup task. LGTM, merge pending CI
3 participants
What changes were proposed in this pull request?
Problem
MutableStat.add(long) is synchronized. OMLockMetrics records four timing stats (read/write lock held and wait times) on every lock acquire and release. Under high read-lock concurrency — when many OM handler threads release a read-lock simultaneously and all rush to call add() — every thread serialises through the same mutex. This creates a thundering-herd contention point that limits OM throughput as thread counts grow.
Approach
Introduce ConcurrentMutableStat, a drop-in subclass of MutableStat that makes add() non-blocking:
Each call accumulates in LongAdder (sum, count) and LongAccumulator (min, max) — all cell-striped via Striped64, so threads write to independent cells with no cross-thread synchronisation.
setChanged() is deferred out of the hot-path add() to avoid concurrent volatile writes from all calling threads.
Pending cells are drained into the parent's running state lazily, only on snapshot(), lastStat(), or toString() — paths that already hold or take the stat's own lock and are called far less frequently than add().
OMLockMetrics and PerformanceMetrics are updated to use ConcurrentMutableStat for their stat fields.
Co-authored with Claude Opus 4.8
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9377
How was this patch tested?
CI: https://github.com/yandrey321/ozone/actions/runs/32549379389
Unit tests (TestConcurrentMutableStat): single-threaded count/mean/min/max correctness, multi-threaded count and extreme-value accuracy, multiple drain cycles.
Integration tests: TestFreon (OmBucketReadWriteFileOps, OmBucketReadWriteKeyOps) — both pass; verifyOMLockMetrics asserts sample counts > 0 for all four lock stats through the full cluster lock/unlock path.
Benchmark
x86
ARM64