RFR (M): JDK-8043607: Add a GC id as a log decoration similar to PrintGCTimeStamps

[Bengt Rutisson]

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
>