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