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