StatisticsHelper: Fix lack of "garbage generated" data (zeros) #25
Conversation
…read And update memory status one final time in stopStatistics() Closes mikemccand#24
|
I'm a little confused on what the bug was here. E.g. were you seeing all 0 for the GC statistics at the end of all benchmark runs? Because we were missing the final |
| @@ -30,6 +31,8 @@ | |||
|
|
|||
| private final AtomicInteger starts = new AtomicInteger(); | |||
|
|
|||
| // TODO reconsider "volatile" everywhere here; we don't need synchronization based on how we use this | |||
|
|
|||
There was a problem hiding this comment.
Seems like we could also make these final?
There was a problem hiding this comment.
A couple booleans but not the pools because they are set in the loop.
| // is the only thread left: | ||
| Thread.currentThread().setDaemon(true); | ||
|
|
||
| if (!hasMemoryPools) |
There was a problem hiding this comment.
Maybe we should simply throw an exception on construction if there are no memory pools? Don't all modern JVMs support this API?
There was a problem hiding this comment.
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.
They will show as "N/A" because hasMemoryPools is false because some of the pool names were not correctly identified. I think there was a separate issue to which I have comments in the issue pertaining to setDaemon etc. and other aspects here. It's been many months for this part and I'm less sure of that. |
StatisticsHelper: Remove attempts to "setDaemon" the stats polling thread
And update memory status one final time in stopStatistics().
Closes #24