RRF: JEP-271: Unified GC Logging

David Lindholm david.lindholm at oracle.com
Mon Nov 23 09:48:49 UTC 2015


Hi Stefan,

Thanks for looking at this. Inline.

On 2015-11-20 17:37, Stefan Johansson wrote:
> 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.

Fixed.

> ---
> 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",

Fixed.

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

Fixed.

> ---
> test/gc/ergonomics/TestDynamicNumberOfGCThreads.java:
>   52     // UseDynamicNumberOfGCThreads and TraceDynamicGCThreads enabled
>
> Comment should be changed to use -Xlog:gc+task=trace as well.

Fixed.

> ---
> 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.

Ok, I did these changes also.

These test changes will be a part of the next webrev sent out by Bengt.


Thanks,
David

> ---
>
> 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
>



More information about the hotspot-gc-dev mailing list