Remove redundant method calls from ThreadStatesGaugeSet#5098
Remove redundant method calls from ThreadStatesGaugeSet#5098magibney wants to merge 1 commit intodropwizard:release/4.2.xfrom
ThreadStatesGaugeSet#5098Conversation
There was a problem hiding this comment.
Pull request overview
This PR attempts to optimize ThreadStatesGaugeSet by eliminating redundant thread dumps and deadlock detection calls. Instead of dumping threads multiple times (once per thread state), the code now dumps threads once and counts states using an EnumMap. It also calls deadlock detection once instead of twice.
Changes:
- Replaced
HashMapwithLinkedHashMapfor deterministic gauge ordering - Removed the
getThreadCount()method that filtered threads by state - Modified
getMetrics()to compute thread info and deadlocked threads once, then create gauges that reference these computed values - Added
ZERO_COUNTconstant for states with no threads - Used
EnumMapto aggregate thread states from a single thread dump
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // do not compute stack traces. | ||
| private final static int STACK_TRACE_DEPTH = 0; | ||
| private final static int[] ZERO_COUNT = new int[1]; |
There was a problem hiding this comment.
The ZERO_COUNT constant is defined as an int array new int[1] but is used as a shared constant. While this works for the intended purpose (returning 0 for thread states with no threads), using a mutable array as a constant is an anti-pattern. If any code were to modify ZERO_COUNT[0], it would affect all thread states that use it. Consider using a constant int value 0 directly, or if an array is needed for the getOrDefault pattern, create a new array each time or document that the array must never be modified.
| ThreadInfo[] threadInfos = getThreadInfo(); | ||
| Set<String> deadlockedThreads = deadlockDetector.getDeadlockedThreads(); | ||
|
|
||
| EnumMap<Thread.State, int[]> byState = new EnumMap<>(Thread.State.class); | ||
| for (ThreadInfo threadInfo : threadInfos) { | ||
| Thread.State tState; | ||
| if (threadInfo != null && (tState = threadInfo.getThreadState()) != null) { | ||
| byState.computeIfAbsent(tState, (k) -> new int[1])[0]++; | ||
| } | ||
| } | ||
|
|
||
| for (final Thread.State state : Thread.State.values()) { | ||
| gauges.put(name(state.toString().toLowerCase(), "count"), | ||
| (Gauge<Object>) () -> getThreadCount(state)); | ||
| (Gauge<Object>) () -> byState.getOrDefault(state, ZERO_COUNT)[0]); | ||
| } | ||
|
|
||
| gauges.put("count", (Gauge<Integer>) threads::getThreadCount); | ||
| gauges.put("daemon.count", (Gauge<Integer>) threads::getDaemonThreadCount); | ||
| gauges.put("peak.count", (Gauge<Integer>) threads::getPeakThreadCount); | ||
| gauges.put("total_started.count", (Gauge<Long>) threads::getTotalStartedThreadCount); | ||
| gauges.put("deadlock.count", (Gauge<Integer>) () -> deadlockDetector.getDeadlockedThreads().size()); | ||
| gauges.put("deadlocks", (Gauge<Set<String>>) deadlockDetector::getDeadlockedThreads); | ||
| gauges.put("deadlock.count", (Gauge<Integer>) deadlockedThreads::size); | ||
| gauges.put("deadlocks", (Gauge<Set<String>>) () -> deadlockedThreads); |
There was a problem hiding this comment.
The gauge lambdas are capturing stale values instead of computing fresh values on each call to getValue(). The threadInfos and deadlockedThreads are computed once when getMetrics() is called, and the gauge lambdas reference these captured values. This means the gauge values will never update after initial registration.
Gauges should compute fresh values each time getValue() is called, as seen in other MetricSet implementations like MemoryUsageGaugeSet and ClassLoadingGaugeSet. The optimization goal of avoiding redundant calls should be achieved differently - perhaps by caching with a TTL as CachedThreadStatesGaugeSet does, or by calling the expensive methods once per getValue() invocation rather than once per metric.
For example, line 67 creates a gauge that returns byState.getOrDefault(state, ZERO_COUNT)[0], which captures the EnumMap computed at line 57. This value will never change even as threads change state.
ThreadStatesGaugeSetcurrently dumps threads 6 times (once for each possible value ofThread.State) and filters out each time byThread.Statevalue.ThreadStatesGaugeSetalso currently invokes deadlock detection twice: once to dump the String representations of deadlocked threads, and again (separately) simply to report the number of deadlocked threads.CachedThreadStatesGaugeSetmitigates the "perThread.State" issue to some extent; but whether or not "cached", there's really no reason to dump threads multiple times, and context of high thread count or complex lock dependencies, these calls can be expensive.This PR eliminates redundant calls (and also puts the gauge in a
LinkedHashMap, for deterministic display).