Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StatisticsHelper: Fix lack of "garbage generated" data (zeros) #25

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

StatisticsHelper: Fix lack of "garbage generated" data (zeros) #25

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
968de85
StatisticsHelper: Remove attempts to "setDaemon" the stats polling th…
dsmiley Sep 20, 2018
1430bb0
Fix memory pool name detection for JDK 11
dsmiley Mar 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 23 additions & 26 deletions src/main/perf/StatisticsHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/** Poached from cometd 2.1.0, Apache 2 license */
/* Poached from cometd 2.1.0, Apache 2 license */

package perf;

Expand All @@ -18,7 +18,8 @@
import java.util.concurrent.atomic.AtomicInteger;

/**
* @version $Revision$ $Date$
* See <a href="https://github.com/cometd/cometd/blob/2.4.x/cometd-java/cometd-java-examples/src/main/java/org/cometd/benchmark/BenchmarkHelper.java"
* >BenchmarkHelper in CometD</a>.
*/
public class StatisticsHelper implements Runnable {

Expand All @@ -30,6 +31,8 @@ public class StatisticsHelper implements Runnable {

private final AtomicInteger starts = new AtomicInteger();

// TODO reconsider "volatile" everywhere here; we don't need synchronization based on how we use this

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we could also make these final?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple booleans but not the pools because they are set in the loop.

private volatile MemoryPoolMXBean youngMemoryPool;

private volatile MemoryPoolMXBean survivorMemoryPool;
Expand Down Expand Up @@ -81,16 +84,15 @@ public StatisticsHelper() {
this.jitCompiler = ManagementFactory.getCompilationMXBean();
this.heapMemory = ManagementFactory.getMemoryMXBean();

// See this (dated) reference:
// https://gist.github.com/szegedi/1474365/ebee66b04aa3468b5e3864945dc48fa3e204548a
List<MemoryPoolMXBean> memoryPools = ManagementFactory.getMemoryPoolMXBeans();
for (MemoryPoolMXBean memoryPool : memoryPools) {
if ("PS Eden Space".equals(memoryPool.getName()) || "Par Eden Space".equals(memoryPool.getName())
|| "G1 Eden".equals(memoryPool.getName())) {
if (memoryPool.getName().contains("Eden")) {
youngMemoryPool = memoryPool;
} else if ("PS Survivor Space".equals(memoryPool.getName()) || "Par Survivor Space".equals(memoryPool.getName())
|| "G1 Survivor".equals(memoryPool.getName())) {
} else if (memoryPool.getName().contains("Survivor")) {
survivorMemoryPool = memoryPool;
} else if ("PS Old Gen".equals(memoryPool.getName()) || "CMS Old Gen".equals(memoryPool.getName())
|| "G1 Old Gen".equals(memoryPool.getName())) {
} else if (memoryPool.getName().contains("Old Gen")) {
oldMemoryPool = memoryPool;
}
}
Expand All @@ -112,23 +114,11 @@ public StatisticsHelper() {

@Override
public void run() {

// So we don't prevent process exit if this
// is the only thread left:
Thread.currentThread().setDaemon(true);

if (!hasMemoryPools)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should simply throw an exception on construction if there are no memory pools? Don't all modern JVMs support this API?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The real concern is the naming assumptions of the pools that can lead to hasMemoryPools becoming false even though the API is present. See my latest commit which fixes it for JDK 11 by being more loose in the detecting the pool name (exact string vs simple contains). Still, the essence of your point stands -- what to do? I think it's a bit much to throw an exception; I'd prefer perhaps a louder warning plus an assert statement even though we don't have tests.

return;

// So we don't prevent process exit if this
// is the only thread left:
Thread.currentThread().setDaemon(true);

long young = youngMemoryPool.getUsage().getUsed();
long survivor = survivorMemoryPool.getUsage().getUsed();
long old = oldMemoryPool.getUsage().getUsed();

if (!polling) {
if (!polling) { // in initial state; don't save deltas
polling = true;
} else {
if (lastYoungUsed <= young) {
Expand Down Expand Up @@ -190,9 +180,11 @@ public boolean startStatistics() {
}
System.err.println("- - - - - - - - - - - - - - - - - - - - ");

scheduler = Executors.newSingleThreadScheduledExecutor();
polling = false;
memoryPoller = scheduler.scheduleWithFixedDelay(this, 0, 250, TimeUnit.MILLISECONDS);
if (hasMemoryPools) {
scheduler = Executors.newSingleThreadScheduledExecutor();
polling = false;
memoryPoller = scheduler.scheduleWithFixedDelay(this, 0, 250, TimeUnit.MILLISECONDS);
}

lastYoungUsed = 0;
if (hasCollectors) {
Expand Down Expand Up @@ -225,8 +217,13 @@ public boolean stopStatistics() {
if (starts.decrementAndGet() > 0)
return false;

memoryPoller.cancel(false);
scheduler.shutdown();
if (scheduler != null) {
memoryPoller.cancel(false);
scheduler.shutdown();
}
if (hasMemoryPools) {
run();// update latest stats
}

System.err.println("- - - - - - - - - - - - - - - - - - - - ");
System.err.println("Statistics Ended at " + new Date());
Expand Down