RRF: JEP-271: Unified GC Logging

[Stefan Johansson]

Hi Bengt,

Overall I think the change is great, a few comments below.

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/
>
src/share/vm/prims/jvmtiEnv.cpp:
  632     if (value == 0) {
  633       LogConfiguration::parse_log_arguments("stdout", "gc", NULL, 
NULL, NULL);
  634     } else {
  635       LogConfiguration::parse_log_arguments("stdout", "gc=off", 
NULL, NULL, NULL);
  636     }

The condition should be changed to "value != 0" to get the same behavior 
as the old code.

---
test/gc/arguments/TestTargetSurvivorRatioFlag:
  217                     // We found start of output emitted by 
-XX:+PrintTenuringDistribution

Please update the comment to say "by -Xlog:gc+age=trace" instead.

---
test/gc/arguments/TestVerifyBeforeAndAfterGCFlags.java:
   45     // VerifyBeforeGC:[Verifying threads heap tenured eden syms 
strs zone dict metaspace chunks hand C-heap code cache ]
   46     public static final String VERIFY_BEFORE_GC_PATTERN = 
"Verifying Before GC";
   47     // VerifyBeforeGC: VerifyBeforeGC: VerifyBeforeGC:
   48     public static final String VERIFY_BEFORE_GC_CORRUPTED_PATTERN 
= "VerifyBeforeGC:(?!\\[Verifying[^]]+\\])";
   49
   50     // VerifyAfterGC:[Verifying threads heap tenured eden syms 
strs zone dict metaspace chunks hand C-heap code cache ]
   51     public static final String VERIFY_AFTER_GC_PATTERN = 
"Verifying After GC";
   52     // VerifyAfterGC: VerifyAfterGC: VerifyAfterGC:
   53     public static final String VERIFY_AFTER_GC_CORRUPTED_PATTERN = 
"VerifyAfterGC:(?!\\[Verifying[^]]+\\])";

Please update the pattern comments to match the new logging better and 
maybe also remove the regexp parts of the corrupted pattern.

   57  new String[] { "-Xlog:gc+verify=debug",
   58                 "-XX:+UseGCLogFileRotation",

UseGCLogFileRotation has been removed and should be removed from the 
test as well.

---
test/gc/ergonomics/TestDynamicNumberOfGCThreads.java:
   52     // UseDynamicNumberOfGCThreads and TraceDynamicGCThreads enabled

Comment should be changed to use -Xlog:gc+task=trace as well.

---
While looking through the testing directories I found some additional 
changes that could be done.

hotspot/test/testlibrary/jdk/test/lib/JDKToolLauncher.java:
PrintGC and PrintGCDetails are used here and should be changed to use 
unified logging.

hotspot/test/runtime/CommandLine/VMOptionsFile/optionfile_unmatched_quote_2:
Xloggc is used here with an unmatched quote, so the test is fine but 
would be nice to change it to another X-option.

---

I will take another round over the hotspot changes next week but so far 
everything else looks good.

> Some tests in the JDK repo have been updated:
> http://cr.openjdk.java.net/~brutisso/JEP-271/review.00/jdk-webrev.00/
>
JDK changes looks good.

Thanks,
Stefan

> 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