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
Hi again, I should have also mentioned that all of the test fixes - the JTreg test changes in the hotspot repo and all of the changes in the jdk repo - were contributed by David Lindholm. Bengt 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
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
Hi Stefan, Thanks for looking at this! 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.
Good catch! Will fix that. I'll go through your comments regarding the tests below with David. Thanks, Bengt
--- 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
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
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. concurrentMarkSweepGeneration.cpp: 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 Remark" 4166, indentation 4175, indentation 4181, indentation 4185, indentation concurrentMarkSweepGeneration.hpp: timerValue() has changed implementation to now return millis. Maybe _timer.milliSeconds() could be used instead. parNewGeneration.cpp: 1184, "Queue overflow" -> "Queue Overflow" collectionSetChooser.cpp: 138, if (Log<LOG_TAGS(gc, liveness)>::is_trace()) should be log_is_ebabled instead. concurrentMark.cpp: 1754, Log<LOG_TAGS(gc, liveness)>::is_trace() should be log_is_ebabled instead. g1CollectedHeap.cpp: 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. g1GCPhaseTimes.cpp: 289 - use log_is_enabled() instead 301 - use log_is_enabled() instead 357 - remove commented out code 365 - remove commented out code g1_globals.hpp: 49 - "gc,remset" -> "gc+remset" g1StringDedupQueue.cpp 158-159, indentation g1StringDedupThread.cpp 156 - use log_is_enabled() instead 158 - use log_is_enabled() instead youngList.cpp 117, indent adjoiningGenerations.cpp Please remove 4 comments inside a call. Rows 170, 186, 212 and 230 gcTaskManager.cpp 470-473 indent gcTaskThread.cpp 81, use log_is_enabled() instead 138, use log_is_enabled() instead 154, use log_is_enabled() instead psAdaptiveSizePolicy.cpp 698-702, this could be on one line. 789-793, this could be on one line. psParallelCompact.cpp 356, indentation. Thanks, David 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
Hi David, On 2015-11-20 18:12, David Lindholm wrote:
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.
I see your point and I went through all usage of the stats tag. (gc, freelist, stats) I removed some, but kept the ones that call "statistics" methods. I think we can rename this in the future, but for now it seemed reasonable to keep the "stats" tag for controlling the output from methods called something with statistics. (gc, stats) I removed all usages of this except for in G1 concurrentMark.cpp where it is again used for methods called something with statistics. (gc, marking, stats, exit) Removed both stats and exit from these calls. (gc, task, stats) Kept these since they refer to "statistics" methods. (gc, survivor, stats) Removed the stats tag and moved to trace level logging.
concurrentMarkSweepGeneration.cpp:
1443, printout talks about foreground collector. I thought the foreground collector was gone?
The CMS foreground collector is gone, so the comment refers to that CMS is falling back to a full GC. I think you are right that the message could be changed now to more clearly state that. However, I would prefer to do that as a separate change later.
2129, indentation.
Done.
CMSCollector::is_cms_reachable() seems to be dead code, this could be removed in a separate patch.
Sent this out as a separate patch.
CMSPhaseAccounting::wallclock_millis() seems to have a different implementation than before, with different functionality.
This is really pretty ugly code. After some thinking and discussion with David we came to the conclusion that the new implementation is actually providing the same functionality as the old one.
CMSCollector::markFromRoots() also talks about the foreground collector.
Same as above. I'd like to change that later.
4058, indentation 4066, "Scavenge Before Remark" should probably be "Pause Scavenge Before Remark" 4166, indentation 4175, indentation 4181, indentation 4185, indentation
Fixed.
concurrentMarkSweepGeneration.hpp:
timerValue() has changed implementation to now return millis. Maybe _timer.milliSeconds() could be used instead.
Unfortunately _timer.milliSeconds() truncates the returned value, so we loose precision. Instead I changed to that timerValue() returns the raw elapsed counter value and then I convert it to seconds in CMSPhaseAccounting, where it is used for a log message.
parNewGeneration.cpp:
1184, "Queue overflow" -> "Queue Overflow"
Fixed.
collectionSetChooser.cpp:
138, if (Log<LOG_TAGS(gc, liveness)>::is_trace()) should be log_is_ebabled instead.
concurrentMark.cpp:
1754, Log<LOG_TAGS(gc, liveness)>::is_trace() should be log_is_ebabled instead.
I changed these and found a few more places where I had used that old pattern too. Updated all of them.
g1CollectedHeap.cpp:
G1CollectedHeap::create_aux_memory_mapper(). TracePageSize is not converted. Will this be done in a later change?
Yes, I would like to change TracePageSize in a later change since it is a bit more complex and not purely a GC flag.
2721, p2i() could be used instead.
Done.
Verifying is now printed on the verify tag. I think that when you enable verification this tag should be enabled.
Good idea. Again, I think I prefer to do that a separate change.
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.
Fixed.
g1GCPhaseTimes.cpp:
289 - use log_is_enabled() instead 301 - use log_is_enabled() instead 357 - remove commented out code 365 - remove commented out code
Fixed.
g1_globals.hpp:
49 - "gc,remset" -> "gc+remset"
Fixed.
g1StringDedupQueue.cpp
158-159, indentation
g1StringDedupThread.cpp
156 - use log_is_enabled() instead 158 - use log_is_enabled() instead
Fixed.
youngList.cpp
117, indent
Fixed.
adjoiningGenerations.cpp
Please remove 4 comments inside a call. Rows 170, 186, 212 and 230
Fixed.
gcTaskManager.cpp
470-473 indent
Fixed.
gcTaskThread.cpp
81, use log_is_enabled() instead 138, use log_is_enabled() instead 154, use log_is_enabled() instead
Fixed.
psAdaptiveSizePolicy.cpp
698-702, this could be on one line. 789-793, this could be on one line.
Fixed.
psParallelCompact.cpp
356, indentation.
Fixed. Thanks, Bengt
Thanks, David
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
Hi Bengt, 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/
This is not a complete review, just a few things I noticed while browsing the webrev. ---------------------------------------------- /src/share/vm/gc/g1/g1StringDedupQueue.cpp 157 log_trace(gc, stringdedup)( 158 " [Queue]\n" 159 " [Dropped: " UINTX_FORMAT "]", _queue->_dropped); Multi-line loging. ---------------------------------------------- src/share/vm/gc/g1/g1StringDedupTable.cpp 573 log_trace(gc, stringdedup)( 574 " [Table]\n" 575 " [Memory Usage: " G1_STRDEDUP_BYTES_FORMAT_NS "]\n" 576 " [Size: " SIZE_FORMAT ", Min: " SIZE_FORMAT ", Max: " SIZE_FORMAT "]\n" 577 " [Entries: " UINTX_FORMAT ", Load: " G1_STRDEDUP_PERCENT_FORMAT_NS ", Cached: " UINTX_FORMAT ", Added: " UINTX_FORMAT ", Removed: " UINTX_FORMAT "]\n" 578 " [Resize Count: " UINTX_FORMAT ", Shrink Threshold: " UINTX_FORMAT "(" G1_STRDEDUP_PERCENT_FORMAT_NS "), Grow Threshold: " UINTX_FORMAT "(" G1_STRDEDUP_PERCENT_FORMAT_NS ")]\n" 579 " [Rehash Count: " UINTX_FORMAT ", Rehash Threshold: " UINTX_FORMAT ", Hash Seed: 0x%x]\n" 580 " [Age Threshold: " UINTX_FORMAT "]", Multi-line logging. ---------------------------------------------- src/share/vm/gc/g1/g1StringDedup*.cpp In general, I think the string dedup logging currently done with log_trace() should be log_debug() instead. cheers, /Per
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
Hi Per, Thanks for looking at this! On 2015-11-23 09:51, Per Liden wrote:
Hi Bengt,
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/
This is not a complete review, just a few things I noticed while browsing the webrev.
---------------------------------------------- /src/share/vm/gc/g1/g1StringDedupQueue.cpp
157 log_trace(gc, stringdedup)( 158 " [Queue]\n" 159 " [Dropped: " UINTX_FORMAT "]", _queue->_dropped);
Multi-line loging.
---------------------------------------------- src/share/vm/gc/g1/g1StringDedupTable.cpp
573 log_trace(gc, stringdedup)( 574 " [Table]\n" 575 " [Memory Usage: " G1_STRDEDUP_BYTES_FORMAT_NS "]\n" 576 " [Size: " SIZE_FORMAT ", Min: " SIZE_FORMAT ", Max: " SIZE_FORMAT "]\n" 577 " [Entries: " UINTX_FORMAT ", Load: " G1_STRDEDUP_PERCENT_FORMAT_NS ", Cached: " UINTX_FORMAT ", Added: " UINTX_FORMAT ", Removed: " UINTX_FORMAT "]\n" 578 " [Resize Count: " UINTX_FORMAT ", Shrink Threshold: " UINTX_FORMAT "(" G1_STRDEDUP_PERCENT_FORMAT_NS "), Grow Threshold: " UINTX_FORMAT "(" G1_STRDEDUP_PERCENT_FORMAT_NS ")]\n" 579 " [Rehash Count: " UINTX_FORMAT ", Rehash Threshold: " UINTX_FORMAT ", Hash Seed: 0x%x]\n" 580 " [Age Threshold: " UINTX_FORMAT "]",
Multi-line logging.
----------------------------------------------
Good catch. Will fix.
src/share/vm/gc/g1/g1StringDedup*.cpp
In general, I think the string dedup logging currently done with log_trace() should be log_debug() instead.
Yes, that makes sense. Will change that. Bengt
cheers, /Per
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
Hi Bengt, I’ve not seen anything egregious as of yet though I’m no where near done. One comment for ostream.cpp. -// log_name comes from -XX:LogFile=log_name, -Xloggc:log_name or +// log_name comes from -XX:LogFile=log_name or // -XX:DumpLoadedClassList=<file_name> IME, -Xloggc:log_name is the most common form used. Not stuck to it, just saying… Regards, Kirk
On Nov 23, 2015, at 9:43 AM, Bengt Rutisson <bengt.rutisson@oracle.com> wrote:
Hi Per,
Thanks for looking at this!
On 2015-11-23 09:51, Per Liden wrote:
Hi Bengt,
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/
This is not a complete review, just a few things I noticed while browsing the webrev.
---------------------------------------------- /src/share/vm/gc/g1/g1StringDedupQueue.cpp
157 log_trace(gc, stringdedup)( 158 " [Queue]\n" 159 " [Dropped: " UINTX_FORMAT "]", _queue->_dropped);
Multi-line loging.
---------------------------------------------- src/share/vm/gc/g1/g1StringDedupTable.cpp
573 log_trace(gc, stringdedup)( 574 " [Table]\n" 575 " [Memory Usage: " G1_STRDEDUP_BYTES_FORMAT_NS "]\n" 576 " [Size: " SIZE_FORMAT ", Min: " SIZE_FORMAT ", Max: " SIZE_FORMAT "]\n" 577 " [Entries: " UINTX_FORMAT ", Load: " G1_STRDEDUP_PERCENT_FORMAT_NS ", Cached: " UINTX_FORMAT ", Added: " UINTX_FORMAT ", Removed: " UINTX_FORMAT "]\n" 578 " [Resize Count: " UINTX_FORMAT ", Shrink Threshold: " UINTX_FORMAT "(" G1_STRDEDUP_PERCENT_FORMAT_NS "), Grow Threshold: " UINTX_FORMAT "(" G1_STRDEDUP_PERCENT_FORMAT_NS ")]\n" 579 " [Rehash Count: " UINTX_FORMAT ", Rehash Threshold: " UINTX_FORMAT ", Hash Seed: 0x%x]\n" 580 " [Age Threshold: " UINTX_FORMAT "]",
Multi-line logging.
----------------------------------------------
Good catch. Will fix.
src/share/vm/gc/g1/g1StringDedup*.cpp
In general, I think the string dedup logging currently done with log_trace() should be log_debug() instead.
Yes, that makes sense. Will change that.
Bengt
cheers, /Per
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
Hi Kirk, On 2015-11-23 14:26, kirk.pepperdine@gmail.com wrote:
Hi Bengt,
I’ve not seen anything egregious as of yet though I’m no where near done.
Thanks for looking at this change!
One comment for ostream.cpp.
-// log_name comes from -XX:LogFile=log_name, -Xloggc:log_name or +// log_name comes from -XX:LogFile=log_name or // -XX:DumpLoadedClassList=<file_name>
IME, -Xloggc:log_name is the most common form used. Not stuck to it, just saying…
Are you saying that it is scary to remove -Xloggc? Yes, I would agree. We've discussed it a bit here. It would be nice to get rid of it since it comes with a bunch of other issues such as log rotation etc. One way to handle the transition would be to map -Xloggc=filename to -Xlog:gc*:filename. It wouldn't be 100% correct and it wouldn't handle the log rotations flags. But maybe it would be a smoother transition for some users. Regards, Bengt
Regards, Kirk
On Nov 23, 2015, at 9:43 AM, Bengt Rutisson <bengt.rutisson@oracle.com <mailto:bengt.rutisson@oracle.com>> wrote:
Hi Per,
Thanks for looking at this!
On 2015-11-23 09:51, Per Liden wrote:
Hi Bengt,
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/
This is not a complete review, just a few things I noticed while browsing the webrev.
---------------------------------------------- /src/share/vm/gc/g1/g1StringDedupQueue.cpp
157 log_trace(gc, stringdedup)( 158 " [Queue]\n" 159 " [Dropped: " UINTX_FORMAT "]", _queue->_dropped);
Multi-line loging.
---------------------------------------------- src/share/vm/gc/g1/g1StringDedupTable.cpp
573 log_trace(gc, stringdedup)( 574 " [Table]\n" 575 " [Memory Usage: " G1_STRDEDUP_BYTES_FORMAT_NS "]\n" 576 " [Size: " SIZE_FORMAT ", Min: " SIZE_FORMAT ", Max: " SIZE_FORMAT "]\n" 577 " [Entries: " UINTX_FORMAT ", Load: " G1_STRDEDUP_PERCENT_FORMAT_NS ", Cached: " UINTX_FORMAT ", Added: " UINTX_FORMAT ", Removed: " UINTX_FORMAT "]\n" 578 " [Resize Count: " UINTX_FORMAT ", Shrink Threshold: " UINTX_FORMAT "(" G1_STRDEDUP_PERCENT_FORMAT_NS "), Grow Threshold: " UINTX_FORMAT "(" G1_STRDEDUP_PERCENT_FORMAT_NS ")]\n" 579 " [Rehash Count: " UINTX_FORMAT ", Rehash Threshold: " UINTX_FORMAT ", Hash Seed: 0x%x]\n" 580 " [Age Threshold: " UINTX_FORMAT "]",
Multi-line logging.
----------------------------------------------
Good catch. Will fix.
src/share/vm/gc/g1/g1StringDedup*.cpp
In general, I think the string dedup logging currently done with log_trace() should be log_debug() instead.
Yes, that makes sense. Will change that.
Bengt
cheers, /Per
Some tests in the JDK repo have been updated: http://cr.openjdk.java.net/~brutisso/JEP-271/review.00/jdk-webrev.00/ <http://cr.openjdk.java.net/%7Ebrutisso/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
On Nov 24, 2015, at 11:51 AM, Bengt Rutisson <bengt.rutisson@oracle.com> wrote:
Hi Kirk,
On 2015-11-23 14:26, kirk.pepperdine@gmail.com <mailto:kirk.pepperdine@gmail.com> wrote:
Hi Bengt,
I’ve not seen anything egregious as of yet though I’m no where near done.
Thanks for looking at this change!
One comment for ostream.cpp.
-// log_name comes from -XX:LogFile=log_name, -Xloggc:log_name or +// log_name comes from -XX:LogFile=log_name or // -XX:DumpLoadedClassList=<file_name>
IME, -Xloggc:log_name is the most common form used. Not stuck to it, just saying…
Are you saying that it is scary to remove -Xloggc? Yes, I would agree. We've discussed it a bit here. It would be nice to get rid of it since it comes with a bunch of other issues such as log rotation etc. One way to handle the transition would be to map -Xloggc=filename to -Xlog:gc*:filename. It wouldn't be 100% correct and it wouldn't handle the log rotations flags. But maybe it would be a smoother transition for some users.
It’s a small change and a very sensible one. The only thing I’m suggesting is that it’s worth considering this or any change in light of the complexity/change tax that projects will have to pay to move to 9. I see two arguments here, depreciate in 9, remove in 10. Or, just pile it onto the tax. I guess you could argue that since GC logging flags are already changing, the later solution works best in both the short and long term. Regards, Kirk
Hi Kirk, On 2015-11-25 08:23, kirk.pepperdine@gmail.com wrote:
On Nov 24, 2015, at 11:51 AM, Bengt Rutisson <bengt.rutisson@oracle.com <mailto:bengt.rutisson@oracle.com>> wrote:
Hi Kirk,
On 2015-11-23 14:26, kirk.pepperdine@gmail.com wrote:
Hi Bengt,
I’ve not seen anything egregious as of yet though I’m no where near done.
Thanks for looking at this change!
One comment for ostream.cpp.
-// log_name comes from -XX:LogFile=log_name, -Xloggc:log_name or +// log_name comes from -XX:LogFile=log_name or // -XX:DumpLoadedClassList=<file_name>
IME, -Xloggc:log_name is the most common form used. Not stuck to it, just saying…
Are you saying that it is scary to remove -Xloggc? Yes, I would agree. We've discussed it a bit here. It would be nice to get rid of it since it comes with a bunch of other issues such as log rotation etc. One way to handle the transition would be to map -Xloggc=filename to -Xlog:gc*:filename. It wouldn't be 100% correct and it wouldn't handle the log rotations flags. But maybe it would be a smoother transition for some users.
It’s a small change and a very sensible one. The only thing I’m suggesting is that it’s worth considering this or any change in light of the complexity/change tax that projects will have to pay to move to 9. I see two arguments here, depreciate in 9, remove in 10. Or, just pile it onto the tax. I guess you could argue that since GC logging flags are already changing, the later solution works best in both the short and long term.
Agreed. I'll look into adding some compatibility mapping for -Xloggc. That and the compatibility mappings for PrintGC and PringGCDetails will be pushed as a separate change after the main change. This is to make sure that we clean out all usages of the old flags from all tests and test harnesses that we have by really removing the flags for a while. Regards, Bengt
Regards, Kirk
Hi everyone, Here is an updated webrev based on the comments from the first review. The changes that have been discussed have only been to the hotspot repo: http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01/ The changes to the JDK repo are the same as in the first webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.00/jdk-webrev.00/ Some partial diffs to make it easier for those who already looked at the first webrev. The unified logging framework was changed in the way it handles the develop logging. Here are the changes I had to do to accommodate the new develop logging: http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01-develop-log... Here are the changes I have made to address comments in the code (based on top of the develop logging changes): http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-code-dif... And here are the changes that David has made to address comment in the tests: http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-test-dif... Enjoy! Bengt 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
On 2015-11-23 18:25, Bengt Rutisson wrote:
Hi everyone,
Here is an updated webrev based on the comments from the first review. The changes that have been discussed have only been to the hotspot repo:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01/
The changes to the JDK repo are the same as in the first webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.00/jdk-webrev.00/
Some partial diffs to make it easier for those who already looked at the first webrev.
The unified logging framework was changed in the way it handles the develop logging. Here are the changes I had to do to accommodate the new develop logging:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01-develop-log...
Looks good.
Here are the changes I have made to address comments in the code (based on top of the develop logging changes):
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-code-dif...
Looks good, just one small thing: src/share/vm/gc/g1/g1StringDedupThread.cpp 158 if (log_is_enabled(Trace, gc, stringdedup)) { All the stringdedup logging has been moved to debug, so this chech should now use Debug.
And here are the changes that David has made to address comment in the tests:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-test-dif...
Looks good. Thanks, Stefan
Enjoy! Bengt
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
Hi Stefan, On 2015-11-24 11:16, Stefan Johansson wrote:
On 2015-11-23 18:25, Bengt Rutisson wrote:
Hi everyone,
Here is an updated webrev based on the comments from the first review. The changes that have been discussed have only been to the hotspot repo:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01/
The changes to the JDK repo are the same as in the first webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.00/jdk-webrev.00/
Some partial diffs to make it easier for those who already looked at the first webrev.
The unified logging framework was changed in the way it handles the develop logging. Here are the changes I had to do to accommodate the new develop logging:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01-develop-log...
Looks good.
Great. Thanks for looking at this!
Here are the changes I have made to address comments in the code (based on top of the develop logging changes):
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-code-dif...
Looks good, just one small thing: src/share/vm/gc/g1/g1StringDedupThread.cpp 158 if (log_is_enabled(Trace, gc, stringdedup)) { All the stringdedup logging has been moved to debug, so this chech should now use Debug.
Good catch! Fixed.
And here are the changes that David has made to address comment in the tests:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-test-dif...
Looks good.
Thanks again for looking at this! Bengt
Thanks, Stefan
Enjoy! Bengt
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
Hi Bengt, My code review for the rest of the files: defNewGeneration.cpp: 517-520: Indentation 766: Shouldn't the tags be "gc, promotion"? collectorPolicy.cpp: 489: Shouldn't the tags be "gc, heap"? genCollectedHeap.cpp: 1150+1152: Indentation generation.cpp: 162: Indentation plab.cpp: 153: Plab_size = " SIZE_FORMAT " desired_net_plab_sz = " SIZE_FORMAT ": Why the capital P in this printout of one variable name but not the other? I think it should be consistent. referenceProcessor.cpp: 515: Indentation referenceProcessor.cpp: 948+953: Indentation metaspace.cpp: 1505: Indentation 1578+1580: Indentation 2695: Indentation runtimeService.cpp: 123-124: Indentation Thanks, David On 2015-11-23 18:25, Bengt Rutisson wrote:
Hi everyone,
Here is an updated webrev based on the comments from the first review. The changes that have been discussed have only been to the hotspot repo:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01/
The changes to the JDK repo are the same as in the first webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.00/jdk-webrev.00/
Some partial diffs to make it easier for those who already looked at the first webrev.
The unified logging framework was changed in the way it handles the develop logging. Here are the changes I had to do to accommodate the new develop logging:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01-develop-log...
Here are the changes I have made to address comments in the code (based on top of the develop logging changes):
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-code-dif...
And here are the changes that David has made to address comment in the tests:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-test-dif...
Enjoy! Bengt
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
Hi David, Thanks for looking at this again! On 2015-11-25 13:29, David Lindholm wrote:
Hi Bengt,
My code review for the rest of the files:
defNewGeneration.cpp:
517-520: Indentation
Fixed.
766: Shouldn't the tags be "gc, promotion"?
Yes. Fixed.
collectorPolicy.cpp:
489: Shouldn't the tags be "gc, heap"?
Yes, fixed.
genCollectedHeap.cpp:
1150+1152: Indentation
generation.cpp:
162: Indentation
Fixed.
plab.cpp:
153: Plab_size = " SIZE_FORMAT " desired_net_plab_sz = " SIZE_FORMAT ": Why the capital P in this printout of one variable name but not the other? I think it should be consistent.
Agreed. Fixed.
referenceProcessor.cpp:
515: Indentation
referenceProcessor.cpp:
948+953: Indentation
metaspace.cpp:
1505: Indentation 1578+1580: Indentation 2695: Indentation
runtimeService.cpp:
123-124: Indentation
Fixed. Thanks, Bengt
Thanks, David
On 2015-11-23 18:25, Bengt Rutisson wrote:
Hi everyone,
Here is an updated webrev based on the comments from the first review. The changes that have been discussed have only been to the hotspot repo:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01/
The changes to the JDK repo are the same as in the first webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.00/jdk-webrev.00/
Some partial diffs to make it easier for those who already looked at the first webrev.
The unified logging framework was changed in the way it handles the develop logging. Here are the changes I had to do to accommodate the new develop logging:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01-develop-log...
Here are the changes I have made to address comments in the code (based on top of the develop logging changes):
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-code-dif...
And here are the changes that David has made to address comment in the tests:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-test-dif...
Enjoy! Bengt
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
Hi everyone, Here are updated webrevs that include the comments on the last review request. HotSpot changes: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/hotspot-webrev.02/ Complete changes: Diff compared to last webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/hotspot-webrev.01-02.... JDK changes: Complete changes: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.02/ Diff compared to last webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.01-02.diff... Except for the comment that were discussed in this email thread there are a couple of other changes: - Most of the implementation parts in gcTraceTime.hpp were moved to gcTraceTime.inline.hpp. - New tests and new code that use logging were updated. - Some tests were changed to use less extensive logging. - A bug gave duplicate timing for Full GCs when going through GenCollectedHeap - The used calculation for G1 old logging could overflow. - The @ignore tag was added back to TestLogging.java. Thanks, Bengt On 2015-11-25 14:00, Bengt Rutisson wrote:
Hi David,
Thanks for looking at this again!
On 2015-11-25 13:29, David Lindholm wrote:
Hi Bengt,
My code review for the rest of the files:
defNewGeneration.cpp:
517-520: Indentation
Fixed.
766: Shouldn't the tags be "gc, promotion"?
Yes. Fixed.
collectorPolicy.cpp:
489: Shouldn't the tags be "gc, heap"?
Yes, fixed.
genCollectedHeap.cpp:
1150+1152: Indentation
generation.cpp:
162: Indentation
Fixed.
plab.cpp:
153: Plab_size = " SIZE_FORMAT " desired_net_plab_sz = " SIZE_FORMAT ": Why the capital P in this printout of one variable name but not the other? I think it should be consistent.
Agreed. Fixed.
referenceProcessor.cpp:
515: Indentation
referenceProcessor.cpp:
948+953: Indentation
metaspace.cpp:
1505: Indentation 1578+1580: Indentation 2695: Indentation
runtimeService.cpp:
123-124: Indentation
Fixed.
Thanks, Bengt
Thanks, David
On 2015-11-23 18:25, Bengt Rutisson wrote:
Hi everyone,
Here is an updated webrev based on the comments from the first review. The changes that have been discussed have only been to the hotspot repo:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01/
The changes to the JDK repo are the same as in the first webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.00/jdk-webrev.00/
Some partial diffs to make it easier for those who already looked at the first webrev.
The unified logging framework was changed in the way it handles the develop logging. Here are the changes I had to do to accommodate the new develop logging:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01-develop-log...
Here are the changes I have made to address comments in the code (based on top of the develop logging changes):
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-code-dif...
And here are the changes that David has made to address comment in the tests:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-test-dif...
Enjoy! Bengt
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
Hi Bengt, These changes looks good now. Reviewed. /David On 2015-12-01 14:09, Bengt Rutisson wrote:
Hi everyone,
Here are updated webrevs that include the comments on the last review request.
HotSpot changes: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/hotspot-webrev.02/
Complete changes:
Diff compared to last webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/hotspot-webrev.01-02....
JDK changes:
Complete changes: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.02/
Diff compared to last webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.01-02.diff...
Except for the comment that were discussed in this email thread there are a couple of other changes:
- Most of the implementation parts in gcTraceTime.hpp were moved to gcTraceTime.inline.hpp. - New tests and new code that use logging were updated. - Some tests were changed to use less extensive logging. - A bug gave duplicate timing for Full GCs when going through GenCollectedHeap - The used calculation for G1 old logging could overflow. - The @ignore tag was added back to TestLogging.java.
Thanks, Bengt
On 2015-11-25 14:00, Bengt Rutisson wrote:
Hi David,
Thanks for looking at this again!
On 2015-11-25 13:29, David Lindholm wrote:
Hi Bengt,
My code review for the rest of the files:
defNewGeneration.cpp:
517-520: Indentation
Fixed.
766: Shouldn't the tags be "gc, promotion"?
Yes. Fixed.
collectorPolicy.cpp:
489: Shouldn't the tags be "gc, heap"?
Yes, fixed.
genCollectedHeap.cpp:
1150+1152: Indentation
generation.cpp:
162: Indentation
Fixed.
plab.cpp:
153: Plab_size = " SIZE_FORMAT " desired_net_plab_sz = " SIZE_FORMAT ": Why the capital P in this printout of one variable name but not the other? I think it should be consistent.
Agreed. Fixed.
referenceProcessor.cpp:
515: Indentation
referenceProcessor.cpp:
948+953: Indentation
metaspace.cpp:
1505: Indentation 1578+1580: Indentation 2695: Indentation
runtimeService.cpp:
123-124: Indentation
Fixed.
Thanks, Bengt
Thanks, David
On 2015-11-23 18:25, Bengt Rutisson wrote:
Hi everyone,
Here is an updated webrev based on the comments from the first review. The changes that have been discussed have only been to the hotspot repo:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01/
The changes to the JDK repo are the same as in the first webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.00/jdk-webrev.00/
Some partial diffs to make it easier for those who already looked at the first webrev.
The unified logging framework was changed in the way it handles the develop logging. Here are the changes I had to do to accommodate the new develop logging:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01-develop-log...
Here are the changes I have made to address comments in the code (based on top of the develop logging changes):
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-code-dif...
And here are the changes that David has made to address comment in the tests:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-test-dif...
Enjoy! Bengt
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
On 2015-12-02 14:07, David Lindholm wrote:
Hi Bengt,
These changes looks good now. Reviewed.
Thanks for looking at this, David! Bengt
/David
On 2015-12-01 14:09, Bengt Rutisson wrote:
Hi everyone,
Here are updated webrevs that include the comments on the last review request.
HotSpot changes: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/hotspot-webrev.02/
Complete changes:
Diff compared to last webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/hotspot-webrev.01-02....
JDK changes:
Complete changes: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.02/
Diff compared to last webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.01-02.diff...
Except for the comment that were discussed in this email thread there are a couple of other changes:
- Most of the implementation parts in gcTraceTime.hpp were moved to gcTraceTime.inline.hpp. - New tests and new code that use logging were updated. - Some tests were changed to use less extensive logging. - A bug gave duplicate timing for Full GCs when going through GenCollectedHeap - The used calculation for G1 old logging could overflow. - The @ignore tag was added back to TestLogging.java.
Thanks, Bengt
On 2015-11-25 14:00, Bengt Rutisson wrote:
Hi David,
Thanks for looking at this again!
On 2015-11-25 13:29, David Lindholm wrote:
Hi Bengt,
My code review for the rest of the files:
defNewGeneration.cpp:
517-520: Indentation
Fixed.
766: Shouldn't the tags be "gc, promotion"?
Yes. Fixed.
collectorPolicy.cpp:
489: Shouldn't the tags be "gc, heap"?
Yes, fixed.
genCollectedHeap.cpp:
1150+1152: Indentation
generation.cpp:
162: Indentation
Fixed.
plab.cpp:
153: Plab_size = " SIZE_FORMAT " desired_net_plab_sz = " SIZE_FORMAT ": Why the capital P in this printout of one variable name but not the other? I think it should be consistent.
Agreed. Fixed.
referenceProcessor.cpp:
515: Indentation
referenceProcessor.cpp:
948+953: Indentation
metaspace.cpp:
1505: Indentation 1578+1580: Indentation 2695: Indentation
runtimeService.cpp:
123-124: Indentation
Fixed.
Thanks, Bengt
Thanks, David
On 2015-11-23 18:25, Bengt Rutisson wrote:
Hi everyone,
Here is an updated webrev based on the comments from the first review. The changes that have been discussed have only been to the hotspot repo:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01/
The changes to the JDK repo are the same as in the first webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.00/jdk-webrev.00/
Some partial diffs to make it easier for those who already looked at the first webrev.
The unified logging framework was changed in the way it handles the develop logging. Here are the changes I had to do to accommodate the new develop logging:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01-develop-log...
Here are the changes I have made to address comments in the code (based on top of the develop logging changes):
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-code-dif...
And here are the changes that David has made to address comment in the tests:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-test-dif...
Enjoy! Bengt
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
Hi, I’m surprised that -Xmx512m is still being used here given that -mx is the fully supported method of specifying max heap size. Regards, Kirk
On Dec 2, 2015, at 2:47 PM, Bengt Rutisson <bengt.rutisson@oracle.com> wrote:
On 2015-12-02 14:07, David Lindholm wrote:
Hi Bengt,
These changes looks good now. Reviewed.
Thanks for looking at this, David!
Bengt
/David
On 2015-12-01 14:09, Bengt Rutisson wrote:
Hi everyone,
Here are updated webrevs that include the comments on the last review request.
HotSpot changes: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/hotspot-webrev.02/
Complete changes:
Diff compared to last webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/hotspot-webrev.01-02....
JDK changes:
Complete changes: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.02/
Diff compared to last webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.01-02.diff...
Except for the comment that were discussed in this email thread there are a couple of other changes:
- Most of the implementation parts in gcTraceTime.hpp were moved to gcTraceTime.inline.hpp. - New tests and new code that use logging were updated. - Some tests were changed to use less extensive logging. - A bug gave duplicate timing for Full GCs when going through GenCollectedHeap - The used calculation for G1 old logging could overflow. - The @ignore tag was added back to TestLogging.java.
Thanks, Bengt
On 2015-11-25 14:00, Bengt Rutisson wrote:
Hi David,
Thanks for looking at this again!
On 2015-11-25 13:29, David Lindholm wrote:
Hi Bengt,
My code review for the rest of the files:
defNewGeneration.cpp:
517-520: Indentation
Fixed.
766: Shouldn't the tags be "gc, promotion"?
Yes. Fixed.
collectorPolicy.cpp:
489: Shouldn't the tags be "gc, heap"?
Yes, fixed.
genCollectedHeap.cpp:
1150+1152: Indentation
generation.cpp:
162: Indentation
Fixed.
plab.cpp:
153: Plab_size = " SIZE_FORMAT " desired_net_plab_sz = " SIZE_FORMAT ": Why the capital P in this printout of one variable name but not the other? I think it should be consistent.
Agreed. Fixed.
referenceProcessor.cpp:
515: Indentation
referenceProcessor.cpp:
948+953: Indentation
metaspace.cpp:
1505: Indentation 1578+1580: Indentation 2695: Indentation
runtimeService.cpp:
123-124: Indentation
Fixed.
Thanks, Bengt
Thanks, David
On 2015-11-23 18:25, Bengt Rutisson wrote:
Hi everyone,
Here is an updated webrev based on the comments from the first review. The changes that have been discussed have only been to the hotspot repo:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01/
The changes to the JDK repo are the same as in the first webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.00/jdk-webrev.00/
Some partial diffs to make it easier for those who already looked at the first webrev.
The unified logging framework was changed in the way it handles the develop logging. Here are the changes I had to do to accommodate the new develop logging:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01-develop-log...
Here are the changes I have made to address comments in the code (based on top of the develop logging changes):
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-code-dif...
And here are the changes that David has made to address comment in the tests:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-test-dif...
Enjoy! Bengt
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
Hi all, Jenny noticed a bug in the heap transition information for G1 (Survivors were reported twice). Here's an updated webrev of the hotspot repo that contains the fix for that: http://cr.openjdk.java.net/~brutisso/JEP-271/review.03/hotspot-webrev.03/ Here is the diff compared to the last webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.03/hotspot-webrev.02-03.... Here is the diff compared to webrev.01 in case someone has reviewed webrev.01 but not webrev.02 yet: http://cr.openjdk.java.net/~brutisso/JEP-271/review.03/hotspot-webrev.01-03.... The JDK repo is unchanged, so thes links are still valid to use for review: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.02/ http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.01-02.diff... Thanks, Bengt On 2015-12-01 14:09, Bengt Rutisson wrote:
Hi everyone,
Here are updated webrevs that include the comments on the last review request.
HotSpot changes: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/hotspot-webrev.02/
Complete changes:
Diff compared to last webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/hotspot-webrev.01-02....
JDK changes:
Complete changes: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.02/
Diff compared to last webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.01-02.diff...
Except for the comment that were discussed in this email thread there are a couple of other changes:
- Most of the implementation parts in gcTraceTime.hpp were moved to gcTraceTime.inline.hpp. - New tests and new code that use logging were updated. - Some tests were changed to use less extensive logging. - A bug gave duplicate timing for Full GCs when going through GenCollectedHeap - The used calculation for G1 old logging could overflow. - The @ignore tag was added back to TestLogging.java.
Thanks, Bengt
On 2015-11-25 14:00, Bengt Rutisson wrote:
Hi David,
Thanks for looking at this again!
On 2015-11-25 13:29, David Lindholm wrote:
Hi Bengt,
My code review for the rest of the files:
defNewGeneration.cpp:
517-520: Indentation
Fixed.
766: Shouldn't the tags be "gc, promotion"?
Yes. Fixed.
collectorPolicy.cpp:
489: Shouldn't the tags be "gc, heap"?
Yes, fixed.
genCollectedHeap.cpp:
1150+1152: Indentation
generation.cpp:
162: Indentation
Fixed.
plab.cpp:
153: Plab_size = " SIZE_FORMAT " desired_net_plab_sz = " SIZE_FORMAT ": Why the capital P in this printout of one variable name but not the other? I think it should be consistent.
Agreed. Fixed.
referenceProcessor.cpp:
515: Indentation
referenceProcessor.cpp:
948+953: Indentation
metaspace.cpp:
1505: Indentation 1578+1580: Indentation 2695: Indentation
runtimeService.cpp:
123-124: Indentation
Fixed.
Thanks, Bengt
Thanks, David
On 2015-11-23 18:25, Bengt Rutisson wrote:
Hi everyone,
Here is an updated webrev based on the comments from the first review. The changes that have been discussed have only been to the hotspot repo:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01/
The changes to the JDK repo are the same as in the first webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.00/jdk-webrev.00/
Some partial diffs to make it easier for those who already looked at the first webrev.
The unified logging framework was changed in the way it handles the develop logging. Here are the changes I had to do to accommodate the new develop logging:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01-develop-log...
Here are the changes I have made to address comments in the code (based on top of the develop logging changes):
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-code-dif...
And here are the changes that David has made to address comment in the tests:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-test-dif...
Enjoy! Bengt
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
Hi Bengt, On 2015-12-07 14:53, Bengt Rutisson wrote:
Hi all,
Jenny noticed a bug in the heap transition information for G1 (Survivors were reported twice). Here's an updated webrev of the hotspot repo that contains the fix for that:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.03/hotspot-webrev.03/
Look good, just one minor thing: src/share/vm/gc/shared/collectorPolicy.cpp 556 log_trace(heap)("2: Minimum young " SIZE_FORMAT " Initial young " SIZE_FORMAT " Maximum young " SIZE_FORMAT, 557 _min_young_size, _initial_young_size, _max_young_size); ... 573 log_trace(heap)("Minimum old " SIZE_FORMAT " Initial old " SIZE_FORMAT " Maximum old " SIZE_FORMAT, 574 _min_old_size, _initial_old_size, _max_old_size); Those two log-statements should use the gc tag, like the statement on line 489.
Here is the diff compared to the last webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.03/hotspot-webrev.02-03....
Here is the diff compared to webrev.01 in case someone has reviewed webrev.01 but not webrev.02 yet:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.03/hotspot-webrev.01-03....
The JDK repo is unchanged, so thes links are still valid to use for review:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.02/ http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.01-02.diff...
This looks good. Thanks, StefanJ
Thanks, Bengt
On 2015-12-01 14:09, Bengt Rutisson wrote:
Hi everyone,
Here are updated webrevs that include the comments on the last review request.
HotSpot changes: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/hotspot-webrev.02/
Complete changes:
Diff compared to last webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/hotspot-webrev.01-02....
JDK changes:
Complete changes: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.02/
Diff compared to last webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.01-02.diff...
Except for the comment that were discussed in this email thread there are a couple of other changes:
- Most of the implementation parts in gcTraceTime.hpp were moved to gcTraceTime.inline.hpp. - New tests and new code that use logging were updated. - Some tests were changed to use less extensive logging. - A bug gave duplicate timing for Full GCs when going through GenCollectedHeap - The used calculation for G1 old logging could overflow. - The @ignore tag was added back to TestLogging.java.
Thanks, Bengt
On 2015-11-25 14:00, Bengt Rutisson wrote:
Hi David,
Thanks for looking at this again!
On 2015-11-25 13:29, David Lindholm wrote:
Hi Bengt,
My code review for the rest of the files:
defNewGeneration.cpp:
517-520: Indentation
Fixed.
766: Shouldn't the tags be "gc, promotion"?
Yes. Fixed.
collectorPolicy.cpp:
489: Shouldn't the tags be "gc, heap"?
Yes, fixed.
genCollectedHeap.cpp:
1150+1152: Indentation
generation.cpp:
162: Indentation
Fixed.
plab.cpp:
153: Plab_size = " SIZE_FORMAT " desired_net_plab_sz = " SIZE_FORMAT ": Why the capital P in this printout of one variable name but not the other? I think it should be consistent.
Agreed. Fixed.
referenceProcessor.cpp:
515: Indentation
referenceProcessor.cpp:
948+953: Indentation
metaspace.cpp:
1505: Indentation 1578+1580: Indentation 2695: Indentation
runtimeService.cpp:
123-124: Indentation
Fixed.
Thanks, Bengt
Thanks, David
On 2015-11-23 18:25, Bengt Rutisson wrote:
Hi everyone,
Here is an updated webrev based on the comments from the first review. The changes that have been discussed have only been to the hotspot repo:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01/
The changes to the JDK repo are the same as in the first webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.00/jdk-webrev.00/
Some partial diffs to make it easier for those who already looked at the first webrev.
The unified logging framework was changed in the way it handles the develop logging. Here are the changes I had to do to accommodate the new develop logging:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01-develop-log...
Here are the changes I have made to address comments in the code (based on top of the develop logging changes):
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-code-dif...
And here are the changes that David has made to address comment in the tests:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-test-dif...
Enjoy! Bengt
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
Hi Stefan, On 2015-12-07 17:46, Stefan Johansson wrote:
Hi Bengt,
On 2015-12-07 14:53, Bengt Rutisson wrote:
Hi all,
Jenny noticed a bug in the heap transition information for G1 (Survivors were reported twice). Here's an updated webrev of the hotspot repo that contains the fix for that:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.03/hotspot-webrev.03/
Look good, just one minor thing: src/share/vm/gc/shared/collectorPolicy.cpp 556 log_trace(heap)("2: Minimum young " SIZE_FORMAT " Initial young " SIZE_FORMAT " Maximum young " SIZE_FORMAT, 557 _min_young_size, _initial_young_size, _max_young_size); ... 573 log_trace(heap)("Minimum old " SIZE_FORMAT " Initial old " SIZE_FORMAT " Maximum old " SIZE_FORMAT, 574 _min_old_size, _initial_old_size, _max_old_size);
Those two log-statements should use the gc tag, like the statement on line 489.
Good point. Fixed.
Here is the diff compared to the last webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.03/hotspot-webrev.02-03....
Here is the diff compared to webrev.01 in case someone has reviewed webrev.01 but not webrev.02 yet:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.03/hotspot-webrev.01-03....
The JDK repo is unchanged, so thes links are still valid to use for review:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.02/ http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.01-02.diff...
This looks good.
Thanks for looking at this! Bengt
Thanks, StefanJ
Thanks, Bengt
On 2015-12-01 14:09, Bengt Rutisson wrote:
Hi everyone,
Here are updated webrevs that include the comments on the last review request.
HotSpot changes: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/hotspot-webrev.02/
Complete changes:
Diff compared to last webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/hotspot-webrev.01-02....
JDK changes:
Complete changes: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.02/
Diff compared to last webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.02/jdk-webrev.01-02.diff...
Except for the comment that were discussed in this email thread there are a couple of other changes:
- Most of the implementation parts in gcTraceTime.hpp were moved to gcTraceTime.inline.hpp. - New tests and new code that use logging were updated. - Some tests were changed to use less extensive logging. - A bug gave duplicate timing for Full GCs when going through GenCollectedHeap - The used calculation for G1 old logging could overflow. - The @ignore tag was added back to TestLogging.java.
Thanks, Bengt
On 2015-11-25 14:00, Bengt Rutisson wrote:
Hi David,
Thanks for looking at this again!
On 2015-11-25 13:29, David Lindholm wrote:
Hi Bengt,
My code review for the rest of the files:
defNewGeneration.cpp:
517-520: Indentation
Fixed.
766: Shouldn't the tags be "gc, promotion"?
Yes. Fixed.
collectorPolicy.cpp:
489: Shouldn't the tags be "gc, heap"?
Yes, fixed.
genCollectedHeap.cpp:
1150+1152: Indentation
generation.cpp:
162: Indentation
Fixed.
plab.cpp:
153: Plab_size = " SIZE_FORMAT " desired_net_plab_sz = " SIZE_FORMAT ": Why the capital P in this printout of one variable name but not the other? I think it should be consistent.
Agreed. Fixed.
referenceProcessor.cpp:
515: Indentation
referenceProcessor.cpp:
948+953: Indentation
metaspace.cpp:
1505: Indentation 1578+1580: Indentation 2695: Indentation
runtimeService.cpp:
123-124: Indentation
Fixed.
Thanks, Bengt
Thanks, David
On 2015-11-23 18:25, Bengt Rutisson wrote:
Hi everyone,
Here is an updated webrev based on the comments from the first review. The changes that have been discussed have only been to the hotspot repo:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01/
The changes to the JDK repo are the same as in the first webrev: http://cr.openjdk.java.net/~brutisso/JEP-271/review.00/jdk-webrev.00/
Some partial diffs to make it easier for those who already looked at the first webrev.
The unified logging framework was changed in the way it handles the develop logging. Here are the changes I had to do to accommodate the new develop logging:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.01-develop-log...
Here are the changes I have made to address comments in the code (based on top of the develop logging changes):
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-code-dif...
And here are the changes that David has made to address comment in the tests:
http://cr.openjdk.java.net/~brutisso/JEP-271/review.01/webrev.00-01-test-dif...
Enjoy! Bengt
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
participants (5)
-
Bengt Rutisson
-
David Lindholm
-
kirk.pepperdine＠gmail.com
-
Per Liden
-
Stefan Johansson