RRF: JEP-271: Unified GC Logging

David Lindholm david.lindholm at oracle.com
Fri Nov 20 17:12:45 UTC 2015

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.


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 
4166, indentation
4175, indentation
4181, indentation
4185, indentation


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


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


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


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


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.


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


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


158-159, indentation


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


117, indent


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


470-473 indent


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


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


356, indentation.


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

More information about the hotspot-gc-dev mailing list