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

[Jesper Wilhelmsson]

I like this version better than the last one :)
Reviewed.
/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
>>>
>>>
>>
>