You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "yandrey321 (via GitHub)" <gi...@apache.org> on 2026/08/22 02:52:27 UTC
[PR] HDDS-9377. Replace MutableStat with lock-free ConcurrentMutableStat in OMLockMetrics and PerformanceMetrics [ozone]
yandrey321 opened a new pull request, #11085:
URL: https://github.com/apache/ozone/pull/11085
## 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.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9377
## How was this patch tested?
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
```
Threads MutableStat µs/burst ConcurrentMutableStat µs/burst Speedup
--------------------------------------------------------------------------------------------
1 29.39 µs 27.46 µs 1.1x
10 744.88 µs 150.38 µs 5.0x
20 1.51 ms 267.03 µs 5.7x
40 2.91 ms 518.87 µs 5.6x
60 8.85 ms 768.37 µs 11.5x
80 7.40 ms 996.14 µs 7.4x
=== Steady-State (continuous load, 500 ms window) ===
Threads MutableStat ops/ms ConcurrentMutableStat ops/ms Speedup
--------------------------------------------------------------------------------------------
1 35.83 k ops/ms 60.30 k ops/ms 1.7x
10 4.40 k ops/ms 538.24 k ops/ms 122.4x
20 4.85 k ops/ms 1.06 G ops/ms 218.4x
40 4.71 k ops/ms 1.45 G ops/ms 308.5x
60 5.42 k ops/ms 2.32 G ops/ms 427.7x
80 5.73 k ops/ms 2.58 G ops/ms 450.9x
```
ARM64
```
Threads MutableStat µs/burst ConcurrentMutableStat µs/burst Speedup
--------------------------------------------------------------------------------------------
1 15.16 µs 14.50 µs 1.0x
10 177.60 µs 235.61 µs 0.8x
20 167.04 µs 378.45 µs 0.4x
40 336.60 µs 1.28 ms 0.3x
60 517.25 µs 1.95 ms 0.3x
80 791.09 µs 2.40 ms 0.3x
=== Steady-State (continuous load, 500 ms window) ===
Threads MutableStat ops/ms ConcurrentMutableStat ops/ms Speedup
--------------------------------------------------------------------------------------------
1 292.66 k ops/ms 190.49 k ops/ms 0.7x
10 10.77 k ops/ms 17.97 k ops/ms 1.7x
20 9.96 k ops/ms 18.10 k ops/ms 1.8x
40 10.22 k ops/ms 16.72 k ops/ms 1.6x
60 12.40 k ops/ms 16.57 k ops/ms 1.3x
80 9.44 k ops/ms 18.16 k ops/ms 1.9x
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9377. Replace MutableStat with lock-free ConcurrentMutableStat in OMLockMetrics and PerformanceMetrics [ozone]
Posted by "ptlrs (via GitHub)" <gi...@apache.org>.
ptlrs commented on code in PR #11085:
URL: https://github.com/apache/ozone/pull/11085#discussion_r3898975338
##########
hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/lock/OMLockMetrics.java:
##########
@@ -37,26 +36,24 @@ public final class OMLockMetrics implements MetricsSource {
private static final String SOURCE_NAME =
OMLockMetrics.class.getSimpleName();
- private final MetricsRegistry registry;
- private final MutableStat readLockWaitingTimeMsStat;
- private final MutableStat readLockHeldTimeMsStat;
- private final MutableStat writeLockWaitingTimeMsStat;
- private final MutableStat writeLockHeldTimeMsStat;
+ private final ConcurrentMutableStat readLockWaitingTimeMsStat;
+ private final ConcurrentMutableStat readLockHeldTimeMsStat;
+ private final ConcurrentMutableStat writeLockWaitingTimeMsStat;
+ private final ConcurrentMutableStat writeLockHeldTimeMsStat;
private OMLockMetrics() {
- registry = new MetricsRegistry(SOURCE_NAME);
- readLockWaitingTimeMsStat = registry.newStat("ReadLockWaitingTime",
+ readLockWaitingTimeMsStat = new ConcurrentMutableStat("ReadLockWaitingTime",
"Time (in milliseconds) spent waiting for acquiring the read lock",
- "Ops", "Time", true);
- readLockHeldTimeMsStat = registry.newStat("ReadLockHeldTime",
+ "Ops", "Time", false);
Review Comment:
@yandrey321 @jojochuang As per this transition of not collecting extended stats, we will now only know the count and averages. We won't know the min and max times that the read and write locks were held or waited upon.
Cases where the p99/p100 values are not sufficiently large to move the avg or p50 will be missed and difficult to track.
Does this hinder future investigations where metrics for these values are needed or are there any alternatives to obtaining these values?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9377. Replace MutableStat with lock-free ConcurrentMutableStat in OMLockMetrics and PerformanceMetrics [ozone]
Posted by "yandrey321 (via GitHub)" <gi...@apache.org>.
yandrey321 commented on PR #11085:
URL: https://github.com/apache/ozone/pull/11085#issuecomment-5486288880
@ptlrs we can enable extended stats, the only gap that we're going to have is std_dev that would be screwed to 0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9377. Replace MutableStat with lock-free ConcurrentMutableStat in OMLockMetrics and PerformanceMetrics [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang merged PR #11085:
URL: https://github.com/apache/ozone/pull/11085
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9377. Replace MutableStat with lock-free ConcurrentMutableStat in OMLockMetrics and PerformanceMetrics [ozone]
Posted by "yandrey321 (via GitHub)" <gi...@apache.org>.
yandrey321 commented on code in PR #11085:
URL: https://github.com/apache/ozone/pull/11085#discussion_r3844869376
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/util/ConcurrentMutableStat.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.util;
+
+import java.util.concurrent.atomic.LongAccumulator;
+import java.util.concurrent.atomic.LongAdder;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.lib.MutableStat;
+import org.apache.hadoop.metrics2.util.SampleStat;
+
+/**
+ * A {@link MutableStat} whose hot-path {@link #add(long)} is non-blocking
+ * under concurrent callers. Each call accumulates in {@link LongAdder} /
+ * {@link LongAccumulator} cells and the pending totals are drained into the
+ * parent's running state lazily — only when metrics are read via
+ * {@link #snapshot}, {@link #lastStat}, or {@link #toString}.
+ *
+ * <p>This avoids the {@code synchronized} contention of the base class when
+ * many threads release locks simultaneously and all attempt to record a
+ * measurement on the same metric instance.
+ */
+public class ConcurrentMutableStat extends MutableStat {
+
+ private final LongAdder pendingSum = new LongAdder();
+ private final LongAdder pendingCount = new LongAdder();
+ /** Cell-striped min accumulator: identity = Long.MAX_VALUE, function = Math::min. */
+ private final LongAccumulator pendingMin = new LongAccumulator(Math::min, Long.MAX_VALUE);
+ /** Cell-striped max accumulator: identity = Long.MIN_VALUE, function = Math::max. */
+ private final LongAccumulator pendingMax = new LongAccumulator(Math::max, Long.MIN_VALUE);
+
+ public ConcurrentMutableStat(String name, String description,
+ String sampleName, String valueName, boolean extended) {
+ super(name, description, sampleName, valueName, extended);
Review Comment:
Good catch, stdev_time would be underestimated with the current implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9377. Replace MutableStat with lock-free ConcurrentMutableStat in OMLockMetrics and PerformanceMetrics [ozone]
Posted by "yandrey321 (via GitHub)" <gi...@apache.org>.
yandrey321 commented on PR #11085:
URL: https://github.com/apache/ozone/pull/11085#issuecomment-5398313145
> @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.
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.
> Additionally, this has some additional memory usages per metrics.
Yes, its a trade off for fixing lock contention.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9377. Replace MutableStat with lock-free ConcurrentMutableStat in OMLockMetrics and PerformanceMetrics [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #11085:
URL: https://github.com/apache/ozone/pull/11085#issuecomment-5413350068
Merged. Thanks for the review @sumitagrawl and PR @yandrey321
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9377. Replace MutableStat with lock-free ConcurrentMutableStat in OMLockMetrics and PerformanceMetrics [ozone]
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #11085:
URL: https://github.com/apache/ozone/pull/11085#discussion_r3843203567
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/util/ConcurrentMutableStat.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.util;
+
+import java.util.concurrent.atomic.LongAccumulator;
+import java.util.concurrent.atomic.LongAdder;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.lib.MutableStat;
+import org.apache.hadoop.metrics2.util.SampleStat;
+
+/**
+ * A {@link MutableStat} whose hot-path {@link #add(long)} is non-blocking
+ * under concurrent callers. Each call accumulates in {@link LongAdder} /
+ * {@link LongAccumulator} cells and the pending totals are drained into the
+ * parent's running state lazily — only when metrics are read via
+ * {@link #snapshot}, {@link #lastStat}, or {@link #toString}.
+ *
+ * <p>This avoids the {@code synchronized} contention of the base class when
+ * many threads release locks simultaneously and all attempt to record a
+ * measurement on the same metric instance.
+ */
+public class ConcurrentMutableStat extends MutableStat {
+
+ private final LongAdder pendingSum = new LongAdder();
+ private final LongAdder pendingCount = new LongAdder();
+ /** Cell-striped min accumulator: identity = Long.MAX_VALUE, function = Math::min. */
+ private final LongAccumulator pendingMin = new LongAccumulator(Math::min, Long.MAX_VALUE);
+ /** Cell-striped max accumulator: identity = Long.MIN_VALUE, function = Math::max. */
+ private final LongAccumulator pendingMax = new LongAccumulator(Math::max, Long.MIN_VALUE);
+
+ public ConcurrentMutableStat(String name, String description,
+ String sampleName, String valueName, boolean extended) {
+ super(name, description, sampleName, valueName, extended);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9377. Replace MutableStat with lock-free ConcurrentMutableStat in OMLockMetrics and PerformanceMetrics [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #11085:
URL: https://github.com/apache/ozone/pull/11085#discussion_r3847159319
##########
hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/lock/OMLockMetrics.java:
##########
@@ -37,26 +36,24 @@ public final class OMLockMetrics implements MetricsSource {
private static final String SOURCE_NAME =
OMLockMetrics.class.getSimpleName();
- private final MetricsRegistry registry;
- private final MutableStat readLockWaitingTimeMsStat;
- private final MutableStat readLockHeldTimeMsStat;
- private final MutableStat writeLockWaitingTimeMsStat;
- private final MutableStat writeLockHeldTimeMsStat;
+ private final ConcurrentMutableStat readLockWaitingTimeMsStat;
+ private final ConcurrentMutableStat readLockHeldTimeMsStat;
+ private final ConcurrentMutableStat writeLockWaitingTimeMsStat;
+ private final ConcurrentMutableStat writeLockHeldTimeMsStat;
private OMLockMetrics() {
- registry = new MetricsRegistry(SOURCE_NAME);
- readLockWaitingTimeMsStat = registry.newStat("ReadLockWaitingTime",
+ readLockWaitingTimeMsStat = new ConcurrentMutableStat("ReadLockWaitingTime",
"Time (in milliseconds) spent waiting for acquiring the read lock",
- "Ops", "Time", true);
- readLockHeldTimeMsStat = registry.newStat("ReadLockHeldTime",
+ "Ops", "Time", false);
Review 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.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/util/ConcurrentMutableStat.java:
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.util;
+
+import java.util.concurrent.atomic.LongAccumulator;
+import java.util.concurrent.atomic.LongAdder;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.lib.MutableStat;
+import org.apache.hadoop.metrics2.util.SampleStat;
+
+/**
+ * A {@link MutableStat} whose hot-path {@link #add(long)} is non-blocking
+ * under concurrent callers. Each call accumulates in {@link LongAdder} /
+ * {@link LongAccumulator} cells and the pending totals are drained into the
+ * parent's running state lazily — only when metrics are read via
+ * {@link #snapshot}, {@link #lastStat}, or {@link #toString}.
+ *
+ * <p>This avoids the {@code synchronized} contention of the base class when
+ * many threads release locks simultaneously and all attempt to record a
+ * measurement on the same metric instance.
+ *
+ * <p><b>Standard deviation accuracy:</b> {@code drainPending()} batches all
+ * pending samples except the min and max into a single
+ * {@code super.add(n, sum)} call. {@link org.apache.hadoop.metrics2.util.SampleStat}
+ * treats a batch as {@code n} identical samples equal to their mean, so the
+ * within-batch variance is lost. The reported standard deviation is therefore
+ * underestimated. Callers that need accurate standard deviation should use
+ * {@link org.apache.hadoop.metrics2.lib.MutableStat} directly, or pass
+ * {@code extended=false} to suppress the stdev metric.
+ */
+public class ConcurrentMutableStat extends MutableStat {
+
+ private final LongAdder pendingSum = new LongAdder();
+ private final LongAdder pendingCount = new LongAdder();
+ /** Cell-striped min accumulator: identity = Long.MAX_VALUE, function = Math::min. */
+ private final LongAccumulator pendingMin = new LongAccumulator(Math::min, Long.MAX_VALUE);
+ /** Cell-striped max accumulator: identity = Long.MIN_VALUE, function = Math::max. */
+ private final LongAccumulator pendingMax = new LongAccumulator(Math::max, Long.MIN_VALUE);
+
+ public ConcurrentMutableStat(String name, String description,
+ String sampleName, String valueName, boolean extended) {
+ super(name, description, sampleName, valueName, extended);
Review Comment:
```suggestion
String sampleName, String valueName) {
super(name, description, sampleName, valueName, false);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org