Request for review (S): 7166894 Add gc cause to Full GC logging for all collectors

[Vitaly Davidovich]

Looks good to me.  I see your point about refactoring the logging code, but
I'd probably at least define the buf size (i.e. 128) as a constant
somewhere and use that, but that's very minor.

Cheers

Sent from my phone
On May 7, 2012 4:05 PM, "Bengt Rutisson" <bengt.rutisson at oracle.com> wrote:

>
> Hi Vitaly and Mikael,
>
> Thanks both of you for looking at this! Here is an updated webrev using
> jio_snprintf:
> http://cr.openjdk.java.net/~brutisso/7166894/webrev.01/
>
> On 2012-05-07 15:45, Vitaly Davidovich wrote:
>
> Yup, I don't think it would simplify the code but rather make any
> potential maintenance easier, I suspect.  For example (and it's a stretch),
> if you wanted to change the size of the message buffer, you'd modify just
> one place.  However, this is very minor so your call.
>
>
> When I went back to look at the G1 code I think that code will benefit
> even more from some abstraction like you suggested. But on the other hand
> there is a lot of logging code that would benefit from that. So, I have to
> think a bit about how much I would like to change. I'll leave it as it is
> for now.
>
> Thanks again,
> Bengt
>
>  Thanks,
>
> Vitaly
>
> Sent from my phone
> On May 7, 2012 9:41 AM, "Bengt Rutisson" <bengt.rutisson at oracle.com>
> wrote:
>
>>
>> Hi Vitaly,
>>
>> Thanks for looking at this!
>>
>> On 2012-05-07 15:31, Vitaly Davidovich wrote:
>>
>> Hi Bengt,
>>
>> The allocation of the buffer to hold the string and sprintf'ing to it is
>> repeated at the places you changed.  Do you think it's worth it to factor
>> that out into a common function or maybe some stack allocated class whose
>> destructor would free the message buffer? Just a thought ...
>>
>>
>> I see your point, but I'm not sure this would actually simplify the code.
>>
>>  Also, probably better to use snprintf instead of sprintf.
>>
>>
>> Good point. I'll fix this. This code fragment is actually copied from
>> existing G1 code, so I'll go back and change that too.
>>
>> Thanks again for looking at it!
>> Bengt
>>
>>  Regards,
>>
>> Vitaly
>>
>> Sent from my phone
>> On May 7, 2012 8:35 AM, "Bengt Rutisson" <bengt.rutisson at oracle.com>
>> wrote:
>>
>>>
>>> Hi all,
>>>
>>> Can I get a couple of reviews of this simple change:
>>> http://cr.openjdk.java.net/~brutisso/7166894/webrev/
>>>
>>> Background:
>>> I recently pushed a similar fix for G1 as "7163848: G1: Log GC Cause for
>>> a GC". That fix adds the GC cause information to all G1 GCs. It was
>>> discussed if we should do this for all collectors and we came to the
>>> conclusion that it would be fairly safe to do it for all Full GCs. These
>>> log messages already contain the text "(System)" or "(System.gc())" when a
>>> System.gc() happens and PrintGCDetails are enabled. Now they will always
>>> contain the cause in parenthesis, even when only PrintGC is enabled.
>>> Hopefully most parsing will work with this.
>>>
>>> Thanks,
>>> Bengt
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20120507/1df79b26/attachment.htm>