RRF: JEP-271: Unified GC Logging
david.lindholm at oracle.com
Fri Nov 20 17:12:45 UTC 2015
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?
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.
4066, "Scavenge Before Remark" should probably be "Pause Scavenge Before
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
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.
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"
156 - use log_is_enabled() instead
158 - use log_is_enabled() instead
Please remove 4 comments inside a call. Rows 170, 186, 212 and 230
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.
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.
> Most code changes are in the hotspot code:
> Some tests in the JDK repo have been updated:
> As with the pre-reviews I have put togther some examples of what the
> new logging looks like:
> 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.
More information about the hotspot-gc-dev