[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-dev
Subject:    Re: RFR (M): JDK-8043607: Add a GC id as a log decoration similar to PrintGCTimeStamps
From:       Bengt Rutisson <bengt.rutisson () oracle ! com>
Date:       2014-06-19 11:36:39
Message-ID: 53A2CB47.7050302 () oracle ! com
[Download RAW message or body]


Hi again,

Been discussing this patch offline with Erik. He had suggested some 
changes but is now fine with the change. I will push this now.

Here's the final webrev:
http://cr.openjdk.java.net/~brutisso/8043607/webrev.08/

Here's the diff compared to the last one:
http://cr.openjdk.java.net/~brutisso/8043607/webrev.07-08.diff/

Thanks Jesper, Erik and Thomas for reviewing it!
Bengt


On 2014-06-12 10:54, Bengt Rutisson wrote:
>
> Hi again,
>
> Erik noticed two things. I had done an unnecessary change in 
> g1CollectedHeap.cpp and there were issues with getting the correct GC 
> ID for logging an aborted concurrent cycle in G1.
>
> I've reverted the unnecessary change in g1CollectedHeap.cpp and I 
> suggest to solve the issue with the aborted concurrent cycle by 
> keeping track of the GC id that we abort and use that for logging.
>
> Here's an updated webrev:
> http://cr.openjdk.java.net/~brutisso/8043607/webrev.07/
>
> Here's the diff copared to the last one:
> http://cr.openjdk.java.net/~brutisso/8043607/webrev.06-07.diff/
>
> Thanks,
> Bengt
>
>
>
> On 2014-06-10 20:24, Thomas Schatzl wrote:
>> Hi,
>>
>> On Tue, 2014-06-10 at 14:37 +0200, Bengt Rutisson wrote:
>>> Hi Thomas,
>>>
>>> Thanks for looking at this again!
>>>> Regarding the test and possible backporting: the test currently 
>>>> assumes
>>>> that gc id printing is on by default. Wouldn't it be better to specify
>>>> +/-PrintGCId in the these tests to make sure it also works with
>>>> different default settings, and possibly one run checking default
>>>> settings?
>>> Good point. I updated the test:
>>> http://cr.openjdk.java.net/~brutisso/8043607/webrev.06/test/gc/logging/TestGCId.java.html 
>>>
>>>
>>> Full new webrev (only the test has changed):
>>> http://cr.openjdk.java.net/~brutisso/8043607/webrev.06/
>>>
>>>> You mentioned somewhere that you want to backport this change to 8u 
>>>> with
>>>> the default value of PrintGCid to be false (iirc) too.
>>> Right. That is my plan. I will need to send out a separate review for
>>> that since I will do a code change (the default value in globals.hpp),
>>> so we can discuss the details during that review. With the new test the
>>> only required change is to switch verify method for the default case.
>> Okay. Looks good to me.
>>
>> Thomas
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]