Skip to content

Commit 3ebd8d3

Browse files
committed
[#2275] Lock-free MemoryUsage accounting via LongAdder
Replace the exclusive-lock byte counter in MemoryUsage with a striped java.util.concurrent.atomic.LongAdder and make percentUsage volatile. increaseUsage/decreaseUsage become lock-free adds; the existing usageLock/setPercentUsage path (listener events, waitForSpace signalling) is entered only when the rounded percentUsage actually changes - at most ~100/percentUsageMinDelta locked updates per limit traversal instead of one exclusive write lock (plus the parent chain's) per message. isFull() becomes a volatile read. waitForSpace and all public APIs are unchanged. Rationale: profiling the previous commit's benchmark showed ~86% of lock wait time in this class - Topic.send -> isFull (read lock), Message.incrementReferenceCount -> increaseUsage (write lock) and decrementReferenceCount -> decreaseUsage (write lock), each recursing into the broker-global SystemUsage parent. That made usage accounting a broker-wide serialization point that destination sharding cannot avoid. The multi-producer degradation curve is eliminated; the shared-topic and uncontended single-producer cases also improve (~+22% / ~+11%). Allocation is unchanged (JMH gc profiler: ~480 vs ~527 B/op, GC count ~0 in both), so the gain is purely lock behaviour. Semantics validation (all pass against this change): ProducerFlowControlTest (7), ProducerFlowControlSendFailTest (8), TopicProducerFlowControlTest (3), CompositeMessageCursorUsageTest (1), QueueMemoryAndStoreUsageCleanupTest (1). Known trade-off: between an add and the locked percent update there is a sub-percentUsageMinDelta staleness window; rapid crossings may coalesce listener events (pairs stay consistent and the locked update recomputes from the live sum, so it self-corrects). StoreUsage and TempUsage still use the base-class locked path and can be converted the same way as a follow-up. Fix setUsage() concurrent safety - delta set instead of LongAdder.reset() LongAdder.reset() is documented as safe only when there are no concurrent updates: a racing increaseUsage/decreaseUsage could be lost outright (or, in narrow cell-sweep interleavings, applied out of program order), permanently corrupting the accounting. Implement setUsage(value) as a single delta add instead: usage.add(value - usage.sum()); Every interleaving with a concurrent update now maps to a legal linearization (the update is preserved, as if ordered after the set). Also rename the parameter to avoid shadowing the field, and document that setUsage - like the historical plain field assignment - does not propagate an adjustment to the parent usage. Tests: adds a sequential setUsage semantics test and a 200-round concurrent drift-bound hammer (16 threads of balanced increase/decrease pairs racing a mid-flight setUsage; final usage must stay within one in-flight op per thread of the set value). Honest note: the hammer did NOT empirically catch the reset() implementation in 200 rounds - a same-thread increase/decrease pair usually lands in the same adder cell, so both are wiped or both survive and the loss window is vanishingly narrow. The fix is justified by LongAdder's documented contract; the test remains as a regression guard on the delta implementation's bound. Validation: MemoryUsageConcurrencyTest (3) and MemoryUsageTest (5) pass; ProducerFlowControlTest, ProducerFlowControlSendFailTest and TopicProducerFlowControlTest (18 total) pass against the patched client. setUsage has no production callers (test/config only), so this is hardening, not a behavior change for the broker runtime. Harden lock-free usage accounting - liveness soak, invariant docs Untimed waitForSpace() has no polling fallback: it blocks on waitForSpaceCondition and depends entirely on a lasting full -> not-full transition reaching the locked setPercentUsage() path. With lock-free accounting that liveness rests on an ordering invariant (every usage.add() is unconditionally followed by maybeUpdatePercent(), so the temporally last mutation of a lasting drop recomputes from the complete sum and signals). This commit: - documents the invariant loudly at both mutation sites in MemoryUsage so a future edit cannot silently break it, and - adds testUntimedWaitForSpaceLivenessSoak: 150 rounds parking untimed waiters at exactly 100% usage, racing balanced increase/decrease churn (usage never drops below full), then issuing the lasting one-byte drop WHILE churn runs; after churn quiesces every waiter must be released. Passes consistently. GC evidence for the review record (send_08_threads, distinctTopics, direct dispatch, JDK 25, -prof gc): per-op allocation is unchanged by the LongAdder change - 467.5 +/- 36.4 B/op stock vs 440.0 +/- 0.2 B/op patched. Absolute alloc rate rises only in proportion to throughput (1.34M -> 5.69M ops/s in the profiled runs); gc.time was 17ms vs 19ms over the measurement window. The change adds no per-operation allocation and no meaningful GC pressure.
1 parent 6f45655 commit 3ebd8d3

3 files changed

Lines changed: 226 additions & 26 deletions

File tree

activemq-client/src/main/java/org/apache/activemq/usage/MemoryUsage.java

Lines changed: 51 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
package org.apache.activemq.usage;
1818

1919
import java.util.concurrent.TimeUnit;
20+
import java.util.concurrent.atomic.LongAdder;
2021

2122
/**
2223
* Used to keep track of how much of something is being used so that a
@@ -28,7 +29,11 @@
2829
*/
2930
public class MemoryUsage extends Usage<MemoryUsage> {
3031

31-
private long usage;
32+
// PROTOTYPE: lock-free usage accounting. The counter is a striped LongAdder so
33+
// increase/decrease never take an exclusive lock; the usageLock is only taken when the
34+
// rounded percentUsage actually changes (at most ~100/percentUsageMinDelta times per
35+
// limit traversal), which preserves listener events and waitForSpace signalling.
36+
private final LongAdder usage = new LongAdder();
3237

3338
public MemoryUsage() {
3439
this(null, null);
@@ -129,12 +134,8 @@ public boolean isFull() {
129134
if (parent != null && parent.isFull()) {
130135
return true;
131136
}
132-
usageLock.readLock().lock();
133-
try {
134-
return percentUsage >= 100;
135-
} finally {
136-
usageLock.readLock().unlock();
137-
}
137+
// percentUsage is volatile; no lock needed for a read.
138+
return percentUsage >= 100;
138139
}
139140

140141
/**
@@ -159,13 +160,14 @@ public void increaseUsage(long value) {
159160
return;
160161
}
161162

162-
usageLock.writeLock().lock();
163-
try {
164-
usage += value;
165-
setPercentUsage(caclPercentUsage());
166-
} finally {
167-
usageLock.writeLock().unlock();
168-
}
163+
// INVARIANT: every usage.add() MUST be followed unconditionally by
164+
// maybeUpdatePercent() in the same method (no early return or throw between them).
165+
// The liveness of untimed waitForSpace() depends on it: the temporally last mutation
166+
// recomputes the percent from a sum that includes all completed updates, so a lasting
167+
// 100% -> <100% transition always reaches the locked setPercentUsage() path, which
168+
// signals waitForSpaceCondition. Breaking this ordering can strand waiters forever.
169+
usage.add(value);
170+
maybeUpdatePercent();
169171

170172
if (parent != null) {
171173
parent.increaseUsage(value);
@@ -182,31 +184,55 @@ public void decreaseUsage(long value) {
182184
return;
183185
}
184186

185-
usageLock.writeLock().lock();
186-
try {
187-
usage -= value;
188-
setPercentUsage(caclPercentUsage());
189-
} finally {
190-
usageLock.writeLock().unlock();
191-
}
187+
// INVARIANT: add() must be followed unconditionally by maybeUpdatePercent()
188+
// (see increaseUsage for the full liveness rationale).
189+
usage.add(-value);
190+
maybeUpdatePercent();
192191

193192
if (parent != null) {
194193
parent.decreaseUsage(value);
195194
}
196195
}
197196

197+
/**
198+
* Fast-path percent maintenance: a dirty compare against the volatile percentUsage; only
199+
* when the rounded percent has actually changed do we take the writeLock and run the
200+
* existing setPercentUsage() (which fires listener events and signals waitForSpace
201+
* waiters). setPercentUsage recomputes from the live sum under the lock, so the last
202+
* writer always stores a fresh value and transient races self-correct on the next update.
203+
*/
204+
private void maybeUpdatePercent() {
205+
if (caclPercentUsage() != percentUsage) {
206+
usageLock.writeLock().lock();
207+
try {
208+
setPercentUsage(caclPercentUsage());
209+
} finally {
210+
usageLock.writeLock().unlock();
211+
}
212+
}
213+
}
214+
198215
@Override
199216
protected long retrieveUsage() {
200-
return usage;
217+
return usage.sum();
201218
}
202219

203220
@Override
204221
public long getUsage() {
205-
return usage;
222+
return usage.sum();
206223
}
207224

208-
public void setUsage(long usage) {
209-
this.usage = usage;
225+
/**
226+
* Sets the usage to the given value. Implemented as a delta adjustment because
227+
* LongAdder.reset() is only safe when there are no concurrent updates - a racing
228+
* increase/decrease could be lost outright. With a delta add, a concurrent update is
229+
* always preserved, equivalent to it linearizing after the set. Note: as with the
230+
* historical field assignment, this does not propagate an adjustment to the parent
231+
* usage.
232+
*/
233+
public void setUsage(long value) {
234+
this.usage.add(value - this.usage.sum());
235+
maybeUpdatePercent();
210236
}
211237

212238
public void setPercentOfJvmHeap(int percentOfJvmHeap) {

activemq-client/src/main/java/org/apache/activemq/usage/Usage.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,9 @@ public abstract class Usage<T extends Usage> implements Service {
4242

4343
protected final ReentrantReadWriteLock usageLock = new ReentrantReadWriteLock();
4444
protected final Condition waitForSpaceCondition = usageLock.writeLock().newCondition();
45-
protected int percentUsage;
45+
// volatile so lock-free hot paths (isFull, percent-change detection) can read it without
46+
// taking the usageLock; all writes still happen under the writeLock via setPercentUsage().
47+
protected volatile int percentUsage;
4648
protected T parent;
4749
protected String name;
4850

activemq-client/src/test/java/org/apache/activemq/usage/MemoryUsageConcurrencyTest.java

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import static org.junit.Assert.assertEquals;
2020
import static org.junit.Assert.assertNotNull;
21+
import static org.junit.Assert.assertTrue;
2122

2223
import java.util.ArrayList;
2324
import java.util.List;
@@ -37,6 +38,177 @@ public class MemoryUsageConcurrencyTest {
3738

3839
private static final Logger LOG = LoggerFactory.getLogger(MemoryUsageConcurrencyTest.class);
3940

41+
/**
42+
* Liveness soak for the untimed waitForSpace(): unlike the timed variants (which poll),
43+
* it blocks on waitForSpaceCondition and depends entirely on the 100% -> below-100%
44+
* transition reaching the locked setPercentUsage() path that signals the condition.
45+
* With lock-free accounting this rests on the invariant that every usage mutation is
46+
* unconditionally followed by a percent re-check, so the temporally last mutation of a
47+
* lasting drop always signals. Each round parks waiters at exactly 100%, races balanced
48+
* churn pairs (usage never drops below full) against the bookkeeping, then issues a
49+
* lasting one-byte drop WHILE churn is still running. After churn quiesces the usage is
50+
* lastingly below the limit, so every waiter must be released.
51+
*/
52+
@Test
53+
public void testUntimedWaitForSpaceLivenessSoak() throws Exception {
54+
final int rounds = 150;
55+
final int waiters = 4;
56+
final int churners = 6;
57+
58+
for (int round = 0; round < rounds; round++) {
59+
final MemoryUsage u = new MemoryUsage();
60+
u.setLimit(100);
61+
u.start();
62+
final AtomicBoolean churnRunning = new AtomicBoolean(true);
63+
final CountDownLatch released = new CountDownLatch(waiters);
64+
final List<Thread> waiterThreads = new ArrayList<>();
65+
final List<Thread> churnThreads = new ArrayList<>();
66+
try {
67+
u.increaseUsage(100); // exactly full
68+
69+
for (int w = 0; w < waiters; w++) {
70+
final Thread t = new Thread(() -> {
71+
try {
72+
u.waitForSpace(); // untimed: no polling fallback
73+
released.countDown();
74+
} catch (InterruptedException ignored) {
75+
}
76+
});
77+
t.setDaemon(true);
78+
waiterThreads.add(t);
79+
t.start();
80+
}
81+
82+
// wait until every waiter is parked on the condition
83+
final long deadline = System.currentTimeMillis() + 5000;
84+
for (Thread t : waiterThreads) {
85+
while (t.getState() != Thread.State.WAITING && System.currentTimeMillis() < deadline) {
86+
Thread.yield();
87+
}
88+
assertEquals("round " + round + " waiter failed to park", Thread.State.WAITING, t.getState());
89+
}
90+
91+
// balanced churn: increase-then-decrease pairs keep usage >= 100 while
92+
// hammering the lock-free percent bookkeeping
93+
for (int c = 0; c < churners; c++) {
94+
final Thread t = new Thread(() -> {
95+
while (churnRunning.get()) {
96+
u.increaseUsage(3);
97+
u.decreaseUsage(3);
98+
}
99+
});
100+
t.setDaemon(true);
101+
churnThreads.add(t);
102+
t.start();
103+
}
104+
Thread.sleep(2);
105+
106+
// the lasting drop below 100%, deliberately concurrent with churn
107+
u.decreaseUsage(1);
108+
109+
Thread.sleep(1);
110+
churnRunning.set(false);
111+
for (Thread t : churnThreads) {
112+
t.join(5000);
113+
}
114+
115+
// usage is now lastingly 99/100: every untimed waiter must have been signalled
116+
assertTrue("round " + round + " untimed waitForSpace waiters not released: " + u,
117+
released.await(10, TimeUnit.SECONDS));
118+
} finally {
119+
churnRunning.set(false);
120+
u.stop();
121+
}
122+
}
123+
}
124+
125+
@Test
126+
public void testSetUsageSequential() {
127+
final MemoryUsage u = new MemoryUsage();
128+
u.setLimit(1000);
129+
u.start();
130+
try {
131+
u.increaseUsage(100);
132+
assertEquals(100, u.getUsage());
133+
u.setUsage(500);
134+
assertEquals(500, u.getUsage());
135+
assertEquals(50, u.getPercentUsage());
136+
u.setUsage(0);
137+
assertEquals(0, u.getUsage());
138+
assertEquals(0, u.getPercentUsage());
139+
} finally {
140+
u.stop();
141+
}
142+
}
143+
144+
/**
145+
* setUsage() racing balanced increase/decrease pairs must leave the final usage within
146+
* one in-flight operation per thread of the set value. Each worker performs complete
147+
* increase(v);decrease(v) pairs, so after joining, the only legal deviations from the
148+
* set target come from pairs that straddle the set's linearization point (or its
149+
* non-atomic LongAdder.sum() sweep): at most one op of at most maxOp per thread, in
150+
* either direction. A setUsage() built on LongAdder.reset() can additionally lose
151+
* concurrent updates outright (reset() is documented as safe only with no concurrent
152+
* updates), allowing drift beyond this bound.
153+
*/
154+
@Test
155+
public void testConcurrentSetUsageDriftBounded() throws Exception {
156+
final int threads = 16;
157+
final int maxOp = 100;
158+
final int rounds = 200;
159+
final long target = 123456;
160+
161+
for (int round = 0; round < rounds; round++) {
162+
final MemoryUsage u = new MemoryUsage();
163+
u.setLimit(1L << 40);
164+
u.start();
165+
final AtomicBoolean running = new AtomicBoolean(true);
166+
final CountDownLatch startLatch = new CountDownLatch(1);
167+
final List<Thread> workers = new ArrayList<>();
168+
try {
169+
for (int t = 0; t < threads; t++) {
170+
final int seed = round * 31 + t;
171+
final Thread w = new Thread(() -> {
172+
final Random r = new Random(seed);
173+
try {
174+
startLatch.await();
175+
} catch (InterruptedException e) {
176+
return;
177+
}
178+
while (running.get()) {
179+
final int v = r.nextInt(maxOp) + 1;
180+
u.increaseUsage(v);
181+
u.decreaseUsage(v);
182+
}
183+
});
184+
w.setDaemon(true);
185+
workers.add(w);
186+
w.start();
187+
}
188+
189+
startLatch.countDown();
190+
Thread.sleep(2);
191+
u.setUsage(target);
192+
Thread.sleep(2);
193+
running.set(false);
194+
for (Thread w : workers) {
195+
w.join(5000);
196+
}
197+
198+
final long drift = u.getUsage() - target;
199+
final long bound = (long) threads * maxOp;
200+
if (Math.abs(drift) > bound) {
201+
LOG.info("Round {} drift {} exceeds bound {} : {}", round, drift, bound, u);
202+
}
203+
assertEquals("round " + round + " drift " + drift + " exceeds per-thread in-flight bound " + bound,
204+
0, Math.abs(drift) > bound ? drift : 0);
205+
} finally {
206+
running.set(false);
207+
u.stop();
208+
}
209+
}
210+
}
211+
40212
@Test
41213
public void testCycle() throws Exception {
42214
final Random r = new Random(0xb4a14);

0 commit comments

Comments
 (0)