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

[Bengt Rutisson]

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