Skip to content

Commit c44651d

Browse files
committed
2 parents 55188bd + bd88df6 commit c44651d

File tree

4 files changed

+273
-15
lines changed

4 files changed

+273
-15
lines changed

src/changes/changes.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ The <action> type attribute can be add,update,fix,remove.
5151
<action type="add" dev="ggregory" due-to="Gary Gregory">Add org.apache.commons.pool3.proxy.CglibProxySource.Builder.</action>
5252
<action type="add" dev="ggregory" due-to="Gary Gregory">Add org.apache.commons.pool3.proxy.JdkProxySource.Builder.</action>
5353
<action type="add" dev="ggregory" due-to="Gary Gregory">Add org.apache.commons.pool3.proxy.AbstractProxySource.</action>
54+
<action type="add" dev="ggregory" due-to="Pratyay, Gary Gregory">Made statistics collection optional in BaseGenericObjectPool #429.</action>
5455
<!-- FIX -->
5556
<action type="fix" dev="ggregory" due-to="Gary Gregory">Remove -nouses directive from maven-bundle-plugin. OSGi package imports now state 'uses' definitions for package imports, this doesn't affect JPMS (from org.apache.commons:commons-parent:80).</action>
5657
<action type="fix" dev="ggregory" due-to="Gary Gregory">Operation on the "idleHighWaterMark" shared variable in "ErodingFactor" class is not atomic [org.apache.commons.pool3.PoolUtils$ErodingFactor] At PoolUtils.java:[line 101] AT_NONATOMIC_OPERATIONS_ON_SHARED_VARIABLE.</action>

src/main/java/org/apache/commons/pool3/impl/BaseGenericObjectPool.java

Lines changed: 48 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -408,6 +408,7 @@ public String toString() {
408408

409409
private volatile SwallowedExceptionListener swallowedExceptionListener;
410410
private volatile boolean messageStatistics;
411+
private volatile boolean collectDetailedStatistics = BaseObjectPoolConfig.DEFAULT_COLLECT_DETAILED_STATISTICS;
411412

412413
/** Additional configuration properties for abandoned object tracking. */
413414
protected volatile AbandonedConfig abandonedConfig;
@@ -835,6 +836,20 @@ public boolean getMessageStatistics() {
835836
return messageStatistics;
836837
}
837838

839+
/**
840+
* Gets whether detailed timing statistics collection is enabled.
841+
* When {@code false}, the pool will not collect detailed timing statistics for
842+
* mean active time, mean idle time, and mean borrow wait time,
843+
* improving performance under high load.
844+
*
845+
* @return {@code true} if detailed statistics collection is enabled,
846+
* {@code false} if disabled for improved performance.
847+
* @since 2.13.0
848+
*/
849+
public boolean getCollectDetailedStatistics() {
850+
return collectDetailedStatistics;
851+
}
852+
838853
/**
839854
* Gets the minimum amount of time an object may sit idle in the pool
840855
* before it is eligible for eviction by the idle object evictor (if any -
@@ -1270,6 +1285,7 @@ protected void setConfig(final BaseObjectPoolConfig<T> config) {
12701285
setEvictionPolicy(policy);
12711286
}
12721287
setEvictorShutdownTimeout(config.getEvictorShutdownTimeoutDuration());
1288+
setCollectDetailedStatistics(config.getCollectDetailedStatistics());
12731289
}
12741290

12751291
/**
@@ -1455,6 +1471,21 @@ public void setMessagesStatistics(final boolean messagesDetails) {
14551471
this.messageStatistics = messagesDetails;
14561472
}
14571473

1474+
/**
1475+
* Sets whether detailed timing statistics collection is enabled.
1476+
* When {@code false}, the pool will not collect detailed timing statistics,
1477+
* improving performance under high load at the cost of reduced monitoring capabilities.
1478+
* <p>
1479+
* This setting affects data collection for mean active time, mean idle time, and mean borrow wait time.
1480+
* </p>
1481+
*
1482+
* @param collectDetailedStatistics whether to collect detailed statistics.
1483+
* @since 2.13.0
1484+
*/
1485+
public void setCollectDetailedStatistics(final boolean collectDetailedStatistics) {
1486+
this.collectDetailedStatistics = collectDetailedStatistics;
1487+
}
1488+
14581489
/**
14591490
* Sets the minimum amount of time an object may sit idle in the pool
14601491
* before it is eligible for eviction by the idle object evictor (if any -
@@ -1738,20 +1769,19 @@ protected void toStringAppendFields(final StringBuilder builder) {
17381769
*/
17391770
final void updateStatsBorrow(final PooledObject<T> p, final Duration waitDuration) {
17401771
borrowedCount.incrementAndGet();
1741-
idleTimes.add(p.getIdleDuration());
1742-
waitTimes.add(waitDuration);
1743-
1744-
// lock-free optimistic-locking maximum
1745-
Duration currentMaxDuration;
1746-
do {
1747-
currentMaxDuration = maxBorrowWaitDuration.get();
1748-
// if (currentMaxDuration >= waitDuration) {
1749-
// break;
1750-
// }
1751-
if (currentMaxDuration.compareTo(waitDuration) >= 0) {
1752-
break;
1753-
}
1754-
} while (!maxBorrowWaitDuration.compareAndSet(currentMaxDuration, waitDuration));
1772+
// Only collect detailed statistics if enabled
1773+
if (collectDetailedStatistics) {
1774+
idleTimes.add(p.getIdleDuration());
1775+
waitTimes.add(waitDuration);
1776+
// lock-free optimistic-locking maximum
1777+
Duration currentMaxDuration;
1778+
do {
1779+
currentMaxDuration = maxBorrowWaitDuration.get();
1780+
if (currentMaxDuration.compareTo(waitDuration) >= 0) {
1781+
break;
1782+
}
1783+
} while (!maxBorrowWaitDuration.compareAndSet(currentMaxDuration, waitDuration));
1784+
}
17551785
}
17561786

17571787
/**
@@ -1762,7 +1792,10 @@ final void updateStatsBorrow(final PooledObject<T> p, final Duration waitDuratio
17621792
*/
17631793
final void updateStatsReturn(final Duration activeTime) {
17641794
returnedCount.incrementAndGet();
1765-
activeTimes.add(activeTime);
1795+
// Only collect detailed statistics if enabled
1796+
if (collectDetailedStatistics) {
1797+
activeTimes.add(activeTime);
1798+
}
17661799
}
17671800

17681801
}

src/main/java/org/apache/commons/pool3/impl/BaseObjectPoolConfig.java

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,19 @@ public abstract class BaseObjectPoolConfig<T> extends BaseObject implements Clon
166166
*/
167167
public static final String DEFAULT_EVICTION_POLICY_CLASS_NAME = DefaultEvictionPolicy.class.getName();
168168

169+
/**
170+
* The default value for the {@code collectDetailedStatistics} configuration
171+
* attribute. When {@code true}, the pool will collect detailed timing statistics
172+
* for monitoring purposes. When {@code false}, detailed statistics collection
173+
* is disabled, improving performance under high load.
174+
* <p>
175+
* This setting affects data collection for mean active time, mean idle time, and mean borrow wait time.
176+
* </p>
177+
*
178+
* @since 2.13.0
179+
*/
180+
public static final boolean DEFAULT_COLLECT_DETAILED_STATISTICS = true;
181+
169182
private boolean lifo = DEFAULT_LIFO;
170183

171184
private boolean fairness = DEFAULT_FAIRNESS;
@@ -203,6 +216,8 @@ public abstract class BaseObjectPoolConfig<T> extends BaseObject implements Clon
203216

204217
private String jmxNameBase = DEFAULT_JMX_NAME_BASE;
205218

219+
private boolean collectDetailedStatistics = DEFAULT_COLLECT_DETAILED_STATISTICS;
220+
206221
/**
207222
* Constructs a new instance.
208223
*/
@@ -224,6 +239,23 @@ public boolean getBlockWhenExhausted() {
224239
return blockWhenExhausted;
225240
}
226241

242+
/**
243+
* Gets the value for the {@code collectDetailedStatistics} configuration attribute
244+
* for pools created with this configuration instance.
245+
* <p>
246+
* This setting affects data collection for mean active time, mean idle time, and mean borrow wait time.
247+
* </p>
248+
*
249+
* @return {@code true} if detailed statistics collection is enabled,
250+
* {@code false} if disabled for improved performance.
251+
* @see GenericObjectPool#getCollectDetailedStatistics()
252+
* @see GenericKeyedObjectPool#getCollectDetailedStatistics()
253+
* @since 2.13.0
254+
*/
255+
public boolean getCollectDetailedStatistics() {
256+
return collectDetailedStatistics;
257+
}
258+
227259
/**
228260
* Gets the value for the {@code timeBetweenEvictionRuns} configuration
229261
* attribute for pools created with this configuration instance.
@@ -478,6 +510,26 @@ public void setBlockWhenExhausted(final boolean blockWhenExhausted) {
478510
this.blockWhenExhausted = blockWhenExhausted;
479511
}
480512

513+
/**
514+
* Sets the value for the {@code collectDetailedStatistics} configuration attribute
515+
* for pools created with this configuration instance. When {@code false}, the pool
516+
* will not collect detailed timing statistics, improving performance under high load
517+
* at the cost of reduced monitoring capabilities.
518+
* <p>
519+
* This setting affects data collection for mean active time, mean idle time, and mean borrow wait time.
520+
* </p>
521+
*
522+
* @param collectDetailedStatistics The new setting of {@code collectDetailedStatistics}
523+
* for this configuration instance.
524+
*
525+
* @see GenericObjectPool#getCollectDetailedStatistics()
526+
* @see GenericKeyedObjectPool#getCollectDetailedStatistics()
527+
* @since 2.13.0
528+
*/
529+
public void setCollectDetailedStatistics(final boolean collectDetailedStatistics) {
530+
this.collectDetailedStatistics = collectDetailedStatistics;
531+
}
532+
481533
/**
482534
* Sets the value for the {@code timeBetweenEvictionRuns} configuration
483535
* attribute for pools created with this configuration instance.
@@ -755,5 +807,7 @@ protected void toStringAppendFields(final StringBuilder builder) {
755807
builder.append(jmxNamePrefix);
756808
builder.append(", jmxNameBase=");
757809
builder.append(jmxNameBase);
810+
builder.append(", collectDetailedStatistics=");
811+
builder.append(collectDetailedStatistics);
758812
}
759813
}

src/test/java/org/apache/commons/pool3/impl/TestBaseGenericObjectPool.java

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,17 @@
1818
package org.apache.commons.pool3.impl;
1919

2020
import static org.junit.jupiter.api.Assertions.assertEquals;
21+
import static org.junit.jupiter.api.Assertions.assertFalse;
22+
import static org.junit.jupiter.api.Assertions.assertTrue;
2123

2224
import java.lang.management.ManagementFactory;
2325
import java.time.Duration;
2426
import java.util.ArrayList;
27+
import java.util.List;
28+
import java.util.concurrent.CountDownLatch;
29+
import java.util.concurrent.ExecutorService;
30+
import java.util.concurrent.Executors;
31+
import java.util.concurrent.Future;
2532
import java.util.concurrent.TimeUnit;
2633
import java.util.concurrent.atomic.AtomicInteger;
2734

@@ -140,4 +147,167 @@ void testJMXRegistrationLatency() {
140147
pools.forEach(GenericObjectPool::close);
141148
}
142149
}
150+
151+
@Test
152+
void testCollectDetailedStatisticsDefault() {
153+
// Test that collectDetailedStatistics defaults to true for backward compatibility
154+
assertTrue(pool.getCollectDetailedStatistics());
155+
}
156+
157+
@Test
158+
void testCollectDetailedStatisticsConfiguration() {
159+
// Test configuration through config object
160+
final GenericObjectPoolConfig<String> config = new GenericObjectPoolConfig<>();
161+
config.setCollectDetailedStatistics(false);
162+
try (GenericObjectPool<String, TestException> testPool = new GenericObjectPool<>(factory, config)) {
163+
assertFalse(testPool.getCollectDetailedStatistics());
164+
}
165+
// Test runtime configuration
166+
pool.setCollectDetailedStatistics(false);
167+
assertFalse(pool.getCollectDetailedStatistics());
168+
pool.setCollectDetailedStatistics(true);
169+
assertTrue(pool.getCollectDetailedStatistics());
170+
}
171+
172+
@Test
173+
void testCollectDetailedStatisticsDisabled() throws Exception {
174+
// Configure pool to disable detailed statistics
175+
pool.setCollectDetailedStatistics(false);
176+
final DefaultPooledObject<String> pooledObject = (DefaultPooledObject<String>) factory.makeObject();
177+
// Record initial values
178+
final long initialActiveTime = pool.getMeanActiveTimeMillis();
179+
final long initialIdleTime = pool.getMeanIdleDuration().toMillis();
180+
final long initialWaitTime = pool.getMeanBorrowWaitTimeMillis();
181+
final long initialMaxWaitTime = pool.getMaxBorrowWaitTimeMillis();
182+
// Update statistics - should be ignored for detailed stats
183+
pool.updateStatsBorrow(pooledObject, Duration.ofMillis(100));
184+
pool.updateStatsReturn(Duration.ofMillis(200));
185+
// Basic counters should still work
186+
assertEquals(1, pool.getBorrowedCount());
187+
assertEquals(1, pool.getReturnedCount());
188+
// Detailed statistics should remain unchanged
189+
assertEquals(initialActiveTime, pool.getMeanActiveTimeMillis());
190+
assertEquals(initialIdleTime, pool.getMeanIdleDuration().toMillis());
191+
assertEquals(initialWaitTime, pool.getMeanBorrowWaitTimeMillis());
192+
assertEquals(initialMaxWaitTime, pool.getMaxBorrowWaitTimeMillis());
193+
}
194+
195+
@Test
196+
void testCollectDetailedStatisticsEnabled() throws Exception {
197+
// Ensure detailed statistics are enabled (default)
198+
pool.setCollectDetailedStatistics(true);
199+
final DefaultPooledObject<String> pooledObject = (DefaultPooledObject<String>) factory.makeObject();
200+
// Update statistics
201+
pool.updateStatsBorrow(pooledObject, Duration.ofMillis(100));
202+
pool.updateStatsReturn(Duration.ofMillis(200));
203+
// All counters should work
204+
assertEquals(1, pool.getBorrowedCount());
205+
assertEquals(1, pool.getReturnedCount());
206+
// Detailed statistics should be updated
207+
assertEquals(200, pool.getMeanActiveTimeMillis());
208+
assertEquals(100, pool.getMeanBorrowWaitTimeMillis());
209+
assertEquals(100, pool.getMaxBorrowWaitTimeMillis());
210+
}
211+
212+
@Test
213+
void testCollectDetailedStatisticsToggling() throws Exception {
214+
final DefaultPooledObject<String> pooledObject = (DefaultPooledObject<String>) factory.makeObject();
215+
// Start with detailed stats enabled
216+
pool.setCollectDetailedStatistics(true);
217+
pool.updateStatsBorrow(pooledObject, Duration.ofMillis(50));
218+
pool.updateStatsReturn(Duration.ofMillis(100));
219+
assertEquals(50, pool.getMeanBorrowWaitTimeMillis());
220+
assertEquals(100, pool.getMeanActiveTimeMillis());
221+
// Disable detailed stats
222+
pool.setCollectDetailedStatistics(false);
223+
pool.updateStatsBorrow(pooledObject, Duration.ofMillis(200));
224+
pool.updateStatsReturn(Duration.ofMillis(300));
225+
// Detailed stats should remain at previous values
226+
assertEquals(50, pool.getMeanBorrowWaitTimeMillis());
227+
assertEquals(100, pool.getMeanActiveTimeMillis());
228+
// Basic counters should continue to increment
229+
assertEquals(2, pool.getBorrowedCount());
230+
assertEquals(2, pool.getReturnedCount());
231+
}
232+
233+
@Test
234+
void testStatsStoreConcurrentAccess() throws Exception {
235+
// Test the lock-free StatsStore implementation under concurrent load
236+
final int numThreads = 10;
237+
final int operationsPerThread = 1000;
238+
final ExecutorService executor = Executors.newFixedThreadPool(numThreads);
239+
final CountDownLatch startLatch = new CountDownLatch(1);
240+
final CountDownLatch completeLatch = new CountDownLatch(numThreads);
241+
final List<Future<Void>> futures = new ArrayList<>();
242+
// Create threads that will concurrently update statistics
243+
for (int i = 0; i < numThreads; i++) {
244+
final int threadId = i;
245+
futures.add(executor.submit(() -> {
246+
try {
247+
final DefaultPooledObject<String> pooledObject = (DefaultPooledObject<String>) factory.makeObject();
248+
// Wait for all threads to be ready
249+
startLatch.await();
250+
// Perform concurrent operations
251+
for (int j = 0; j < operationsPerThread; j++) {
252+
pool.updateStatsBorrow(pooledObject, Duration.ofMillis(threadId * 10 + j));
253+
pool.updateStatsReturn(Duration.ofMillis(threadId * 20 + j));
254+
}
255+
} catch (Exception e) {
256+
throw new RuntimeException(e);
257+
} finally {
258+
completeLatch.countDown();
259+
}
260+
return null;
261+
}));
262+
}
263+
// Start all threads simultaneously
264+
startLatch.countDown();
265+
// Wait for completion
266+
assertTrue(completeLatch.await(30, TimeUnit.SECONDS), "Concurrent test should complete within 30 seconds");
267+
// Verify no exceptions occurred
268+
for (Future<Void> future : futures) {
269+
future.get(); // Will throw if there was an exception
270+
}
271+
// Verify that statistics were collected (exact values may vary due to race conditions)
272+
assertEquals(numThreads * operationsPerThread, pool.getBorrowedCount());
273+
assertEquals(numThreads * operationsPerThread, pool.getReturnedCount());
274+
// Mean values should be reasonable (not zero or wildly incorrect)
275+
assertTrue(pool.getMeanActiveTimeMillis() >= 0);
276+
assertTrue(pool.getMeanBorrowWaitTimeMillis() >= 0);
277+
assertTrue(pool.getMaxBorrowWaitTimeMillis() >= 0);
278+
executor.shutdown();
279+
assertTrue(executor.awaitTermination(5, TimeUnit.SECONDS));
280+
}
281+
282+
@Test
283+
void testStatsStoreCircularBuffer() throws Exception {
284+
// Test that StatsStore properly handles circular buffer behavior
285+
final DefaultPooledObject<String> pooledObject = (DefaultPooledObject<String>) factory.makeObject();
286+
// Fill beyond the cache size (100) to test circular behavior
287+
final int cacheSize = 100; // BaseGenericObjectPool.MEAN_TIMING_STATS_CACHE_SIZE
288+
for (int i = 0; i < cacheSize + 50; i++) {
289+
pool.updateStatsBorrow(pooledObject, Duration.ofMillis(i));
290+
pool.updateStatsReturn(Duration.ofMillis(i * 2));
291+
}
292+
// Statistics should still be meaningful after circular buffer wrapping
293+
assertTrue(pool.getMeanActiveTimeMillis() > 0);
294+
assertTrue(pool.getMeanBorrowWaitTimeMillis() > 0);
295+
assertTrue(pool.getMaxBorrowWaitTimeMillis() > 0);
296+
// The mean should reflect recent values, not all historical values
297+
// (exact assertion depends on circular buffer implementation)
298+
assertTrue(pool.getMeanBorrowWaitTimeMillis() >= 50); // Should be influenced by recent higher values
299+
}
300+
301+
@Test
302+
void testDetailedStatisticsConfigIntegration() {
303+
// Test that config property is properly applied during pool construction
304+
final GenericObjectPoolConfig<String> config = new GenericObjectPoolConfig<>();
305+
config.setCollectDetailedStatistics(false);
306+
try (GenericObjectPool<String, TestException> testPool = new GenericObjectPool<>(factory, config)) {
307+
assertFalse(testPool.getCollectDetailedStatistics(), "Pool should respect collectDetailedStatistics setting from config");
308+
// Test that toString includes the new property
309+
final String configString = config.toString();
310+
assertTrue(configString.contains("collectDetailedStatistics"), "Config toString should include collectDetailedStatistics property");
311+
}
312+
}
143313
}

0 commit comments

Comments
 (0)