RRF: JEP-271: Unified GC Logging

[David Lindholm]

Hi Bengt,

This is great work. The code becomes much cleaner in many places. So far 
I have reviewed cms, g1 and parallel.

However, I have some comments:

First, a general comment, I would like to remove the stats tag. I think 
most of what we log is statistics, and I don't think this tag adds value.

concurrentMarkSweepGeneration.cpp:

1443, printout talks about foreground collector. I thought the 
foreground collector was gone?
2129, indentation.
CMSCollector::is_cms_reachable() seems to be dead code, this could be 
removed in a separate patch.
CMSPhaseAccounting::wallclock_millis() seems to have a different 
implementation than before, with different functionality.
CMSCollector::markFromRoots() also talks about the foreground collector.
4058, indentation
4066, "Scavenge Before Remark" should probably be "Pause Scavenge Before 
Remark"
4166, indentation
4175, indentation
4181, indentation
4185, indentation

concurrentMarkSweepGeneration.hpp:

timerValue() has changed implementation to now return millis. Maybe 
_timer.milliSeconds() could be used instead.

parNewGeneration.cpp:

1184, "Queue overflow" -> "Queue Overflow"

collectionSetChooser.cpp:

138, if (Log<LOG_TAGS(gc, liveness)>::is_trace()) should be 
log_is_ebabled instead.

concurrentMark.cpp:

1754, Log<LOG_TAGS(gc, liveness)>::is_trace() should be log_is_ebabled 
instead.

g1CollectedHeap.cpp:

G1CollectedHeap::create_aux_memory_mapper(). TracePageSize is not 
converted. Will this be done in a later change?
2721, p2i() could be used instead.
Verifying is now printed on the verify tag. I think that when you enable 
verification this tag should be enabled.
2951, indentation
3220, indentation
3674, tm5 seems like a strange name
print_termination_stats_hdr(), I don't think you have to du the check in 
the beginning of this function, it contains just 4 printouts.
5713 - should be 1 row, not 2.
5729 - should be 1 row, not 2.
5740 - should be 1 row, not 2.
5750 - should be 1 row, not 2.

g1GCPhaseTimes.cpp:

289 - use log_is_enabled() instead
301 - use log_is_enabled() instead
357 - remove commented out code
365 - remove commented out code

g1_globals.hpp:

49 - "gc,remset" -> "gc+remset"

g1StringDedupQueue.cpp

158-159, indentation

g1StringDedupThread.cpp

156 - use log_is_enabled() instead
158 - use log_is_enabled() instead

youngList.cpp

117, indent

adjoiningGenerations.cpp

Please remove 4 comments inside a call. Rows 170, 186, 212 and 230

gcTaskManager.cpp

470-473 indent

gcTaskThread.cpp

81, use log_is_enabled() instead
138, use log_is_enabled() instead
154, use log_is_enabled() instead

psAdaptiveSizePolicy.cpp

698-702, this could be on one line.
789-793, this could be on one line.

psParallelCompact.cpp

356, indentation.


Thanks,
David

On 2015-11-19 16:29, Bengt Rutisson wrote:
>
> Hi everyone,
>
> After three pre-reviews it is time for the fist official review 
> request for JEP-271 Unified GC Logging.
>
> http://openjdk.java.net/jeps/271
>
> Most code changes are in the hotspot code:
> http://cr.openjdk.java.net/~brutisso/JEP-271/review.00/webrev.00/
>
> Some tests in the JDK repo have been updated:
> http://cr.openjdk.java.net/~brutisso/JEP-271/review.00/jdk-webrev.00/
>
> As with the pre-reviews I have put togther some examples of what the 
> new logging looks like:
> http://cr.openjdk.java.net/~brutisso/JEP-271/review.00/compare.html
>
> The intent is that this should cover the bulk of the logging changes. 
> There will most definitely be some follow up changes where we fix 
> details in the log messages etc.
>
> Among many other old logging flags this changeset removes the two 
> flags PringGC and PrintGCDetails. These two will be added back with a 
> follow up changeset, but when they are added back they will be marked 
> as deprecated.
>
> The reason for first removing them and then adding them back is to get 
> testing without these flags. That way we will know that we clean out 
> all usages of these flags from our testing.
>
> Thanks,
> Bengt