RRF: JEP-271: Unified GC Logging

Stefan Johansson stefan.johansson at oracle.com
Mon Dec 7 16:46:24 UTC 2015 

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