RFR: JDK-8152952: Allow G1 phase logging to use individual number of threads
thomas.schatzl at oracle.com
Wed Mar 30 09:15:49 UTC 2016
On Tue, 2016-03-29 at 14:03 +0200, Bengt Rutisson wrote:
> Hi everyone,
> Could I have a couple of reviews for this change?
> Currently if you run with UseDynamicNumberOfGCThreads you can
> potentially get a different number of worker threads each GC. There
> are improvements coming where we want to select a different number of
> worker threads for individual phases. The G1GCPhaseTimes and
> WorkerDataArray structures need to support this.
> The proposed patch sets all slots in the WorkerDataArray to an
> uninitialized value and then only print any values that have actually
> been set for that phase.
> The patch also extends the log message about the number for worker
> threads to also say how many it could potentially have used. And it
> also fixes a missing space in the level 3 and level 4 indentation.
> After applying this patch and running with -Xlog:gc*,phases*=trace
> you get output like:
>[0,581s][info][gc,task ] GC(0) GC Workers: using 2 out of 23
> [0,588s][info][gc,phases] GC(0) Evacuate Collection Set: 5,0ms
> [0,588s][trace][gc,phases] GC(0) GC Worker Start (ms): Min:
> 580,9, Avg: 580,9, Max: 580,9, Diff: 0,0
It would be useful to have the information about the number of threads
used for every top-level WorkerDataArray. That might differ for every
phase in the future. Now it does not matter, because at the moment
every thread at least sets the time spent to zero (i.e. is forced to),
but that will not be the case later.
Not working on something is different to taking "zero" time for it.
My suggestion is to add a ", Workers: X" column to the summary output, like
> [0,588s][debug][gc,phases ] GC(0) Ext Root Scanning (ms):
> Min: 1,7, Avg: 1,7, Max: 1,8, Diff: 0,0, Sum: 3,5
Min: 1,7, Avg: 1,7, Max: 1,8, Diff: 0,0, Sum: 3,5, Workers: 2
> [0,589s][trace][gc,phases,task] GC(0)
> 1,8 1,7 - - - - - - - - - - - - - - -
> [0,588s][trace][gc,phases,task] GC(0)
> 580,9 580,9 - - - - - - - - - - - - - - -
- g1GCPhaseTimes.cpp:109: typo in ASSERT_PHASE_UNINITILAIZED, should be
- ignore the following if wanted: in g1GCPhaseTimes.cpp:127 the if
-clause is structured to have only the asserts in the if-part. I would
prefer if the code that performs some useful work would be first, i.e.
the condition reversed. As mentioned, ymmv.
- I would kind of prefer if ADD_WORKER_KNOWN time were a method instead
of a macro. In case of an uninitialized value that method could just
return 0.0, which would be okay for this addition.
- in WorkerDataArray::WDAPrinter::details, the "unknown value" imo does
not need to be padded out to five spaces or so. I think the main
motivation for this has been to show the values of the different phases
of the evacuation phase in the same column.
The details() method for the size_t values is not aligned at all.
The main reason is waste of space. Not sure here, in doubt keep it.
- I would prefer if the #include "memory/resourceArea.hpp" were put
next to the other includes, of course guarded by the define.
- in workerDataArray.hpp, at the definitions of sum() and average() it
might be useful to mention what impact on sum/average uninitialized
- I would prefer, if a phase has no data, that this would be detected
automatically and the phase either not printed, or (preferably)
indicated that it has not been executed. (I could also live with a
solution where the programmer can choose what happens when there is no
data. That would probably also remove the dependencies on a lot of
other components, since as of right now there is quite a bit of
checking for particular circumstances in G1GCPhaseTimes::print()).
I do not really like the solution based on this change presented for
JDK-8152428 that the programmer is responsible for explicitly
specifying that a phase has not been executed. This will be forgotten,
and only causes unnecessary failures potentially long after a change
has been committed.
The code to print has to iterate over all elements to sum/average them
up anyway, so it already knows that there is no data for a particular
The reason why I prefer an indication that a phase has not been
executed is that instead of a missing line that a missing line easy to
overlook, while a "not executed" line is much more visible.
More information about the hotspot-gc-dev