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

[Bengt Rutisson]

On 2014-06-09 15:33, Jesper Wilhelmsson wrote:
> I like this version better than the last one :)
> Reviewed.

Thanks, Jesper!

Bengt

> /Jesper
>
> Bengt Rutisson skrev 9/6/14 14:04:
>>
>> Hi again,
>>
>> Another updated webrev:
>> http://cr.openjdk.java.net/~brutisso/8043607/webrev.05/
>>
>> Mainly two changes compared to the previous version. I had to remove 
>> the assert
>> hat we only increment the GC id during a safepoint. That is not true 
>> for CMS,
>> but I remember us looking thoroughly at that when we introduced the 
>> GC id for
>> JFR and we came to the conclusion that it was safe. Anyway, this is not
>> different than before, so I will just leave it out of this change for 
>> now.
>>
>> The other thing is that C++ does not guarantee the order of how 
>> static instances
>> are initialized. Since the parallel collector uses static instances of
>> everything, including the GCTracer we may get conflicts with the 
>> statically
>> initialized GCId::UNDEFINED value. To work around this I changed to a 
>> method
>> called GCId::undefnied() that returns the undefined GCId. That made 
>> it possible
>> to have an instance method on GCId to check if it is valid. Using 
>> that method
>> (GCId::is_undefined()) removed the need of overloading the equals 
>> operators.
>>
>> Here's a diff compared to the last webrev:
>> http://cr.openjdk.java.net/~brutisso/8043607/webrev.04-05.diff/
>>
>> Thanks,
>> Bengt
>>
>>
>> On 2014-06-05 22:58, Bengt Rutisson wrote:
>>>
>>> Hi Thomas,
>>>
>>> Thanks for looking at this!
>>>
>>> On 6/5/14 4:28 PM, Thomas Schatzl wrote:
>>>> Hi Bengt,
>>>>
>>>> On Wed, 2014-06-04 at 17:06 +0200, Bengt Rutisson wrote:
>>>>> Hi Erik,
>>>>>
>>>>> On 5/28/14 2:29 PM, Erik Helin wrote:
>>>>>> Hi Bengt,
>>>>>>
>>>>>> just a first quick general comment: would it make sense for GCId 
>>>>>> to be
>>>>>> in its own file or class, so the GC logging don't have to depend on
>>>>>> the GC trace framework?
>>>>> Good point.
>>>>>
>>>>> Here is an updated webrev where I separeted out GCId to its own file:
>>>>> http://cr.openjdk.java.net/~brutisso/8043607/webrev.02/
>>>>>
>>>>> Here's just the diff compared to the earlier version:
>>>>> http://cr.openjdk.java.net/~brutisso/8043607/webrev.01-02.diff/
>>>>>
>>>>> Jesper, the updated webrev also contains the spelling mistake you
>>>>> noticed. Thanks for catching that!
>>>>>
>>>>   two minor comments:
>>>>
>>>> - is it useful to check whether we are at a safepoint and running 
>>>> in the
>>>> VM thread when incrementing the gc id?
>>>
>>> Good idea. Added an assert for that.
>>>
>>>>
>>>> - also it would be nice to have some test case that checks for 
>>>> presence
>>>> of the numbers in log messages (and their absence if disabled).
>>>
>>> Yes, good point. I added a simple test.
>>>
>>> Here's an updated webrev:
>>> http://cr.openjdk.java.net/~brutisso/8043607/webrev.04/
>>>
>>> And here is the diff compared to the previous version:
>>> http://cr.openjdk.java.net/~brutisso/8043607/webrev.03-04.diff/
>>>
>>> Note that I also found a minor bug. The peek() method should not use 
>>> "+1" the
>>> _next_id is already  the next id, just like the name suggests ;). 
>>> This only
>>> affect CMS and I noticed the problem since some IDs could sometimes be
>>> skipped. Now it works fine for CMS too.
>>>
>>> I also had to fix includes to make it compile without precompiled 
>>> headers.
>>>
>>> Thanks,
>>> Bengt
>>>
>>>>
>>>> Thanks,
>>>>    Thomas
>>>>
>>>>
>>>
>>