RRF: JEP-271: Unified GC Logging

Mon Dec 7 13:53:28 UTC 2015

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

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

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.diff/ 
>
>
>
> 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-logging 
>>>>
>>>>
>>>> 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-diff 
>>>>
>>>>
>>>> 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-diff 
>>>>
>>>>
>>>> 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
>>>>
>>>
>>
>