RRF: JEP-271: Unified GC Logging

Bengt Rutisson bengt.rutisson at oracle.com
Tue Dec 8 09:14:13 UTC 2015 

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

More information about the hotspot-gc-dev mailing list