Review request (m): 7160728: Introduce an extra logging level for G1 logging

[Kirk Pepperdine]

Sorry to be late to the game for this one but IMHO that instead of using log levels a better implementation would be a feature by feature binary switch. Quite often I find that I'm interested in one or two entries in the log but with levels I'm going to have to eat the entire elephant to get to them. Also, how do you make a decision as to what is fine, finer and finest? GC logging is in a messy enough state as it is. I'd hate to further add to that mess by not having a well thought out deliberate strategy moving forward

Regards,
Kirk

On 2012-04-13, at 2:24 AM, Tony Printezis wrote:

> Bengt,
> 
> Looks good, ship it.
> 
> Tony
> 
> On 04/12/2012 07:41 PM, Bengt Rutisson wrote:
>> 
>> Hi all,
>> 
>> Thanks Stefan, Jesper and Tony for looking at this!
>> 
>> Here is an updated webrev that I hope include all of your comments:
>> http://cr.openjdk.java.net/~brutisso/7160728/webrev.01/
>> 
>> If I don't hear any complaints I'll go ahead and push this tomorrow.
>> 
>> Thanks again for the reivews!
>> Bengt
>> 
>> 
>> On 2012-04-12 22:21, Bengt Rutisson wrote:
>>> 
>>> Tony,
>>> 
>>> Thanks for looking at this!
>>> 
>>> On 2012-04-12 21:00, Tony Printezis wrote:
>>>> Bengt,
>>>> 
>>>> Thanks for doing this. It looks good. Some minor comments:
>>>> 
>>>> g1CollectedHeap.cpp
>>>> 
>>>> 3617     TraceTime t(verbose_str, G1Log::fine()&&  !G1Log::finer(), true, gclog_or_tty);
>>>> 
>>>> Is it worth introducing something like G1Log::fine_only() which evaluates to _level == level_fine?
>>> 
>>> I think this is the only place where it appears, so personally I don't really think it is worth it. I hope that we can clean this up a bit further in one of the other logging projects. Unless you object strongly, I'll leave it as it is.
>>> 
>>>> 
>>>> g1CollectorPolicy.cpp
>>>> 
>>>> 1029     if (G1Log::finest()) {
>>>> 1030       buf.append("  %3.1lf", val);
>>>> 1031     }
>>>> 
>>>> Do you want to change %3.1lf to %.1lf to be consistent with what you have for the avg / min / etc? (and %3.1lf should be equivalent to %.1lf, right?)
>>> 
>>> Good point. Fixed.
>>> 
>>>> 
>>>> g1_globals.hpp
>>>> 
>>>> 28 #include "gc_implementation/g1/g1Log.hpp" // make sure the logging module is available to all g1 modules
>>>> 
>>>> I would actually skip this and only include it in the appropriate files (which will all be .cpp files) to avoid getting it compiled for everything else.
>>> 
>>> OK. Done.
>>> 
>>>> universe.cpp:
>>>> 
>>>> 896     G1Log::init();
>>>> 
>>>> I would actually initialize this in G1CollectedHeap::initialize(). Do we do any logging before that so that we need to initialize it earlier?
>>> 
>>> Yes, I was thinking about where to initialize it. I'll move it to G1CollectedHeap::initialize().
>>> 
>>>> g1Log.hpp
>>>> 
>>>>  31   typedef enum {
>>>>  32     level_none,
>>>>  33     level_fine,
>>>>  34     level_finer,
>>>>  35     level_finest
>>>>  36   } LogLevel;
>>>> 
>>>> I think the convention for enums is: LevelNone, LevelFine, LevelFiner, LevelFinest?
>>> 
>>> I don't think hotspot is really consistent about the naming for enums, but I'll change to your suggestion.
>>> 
>>>> 
>>>> g1Log.cpp
>>>> 
>>>> Is it worth turning G1LogLevel into lowercase to make the comparison case-insensitive?
>>> 
>>> I don't think we need to have them case insensitive. The command line option are case sensitive.
>>> 
>>> Thanks again for looking at this. I'll send out an updated webrev shortly.
>>> 
>>> Bengt
>>> 
>>>> 
>>>> Tony
>>>> 
>>>> 
>>>> On 04/11/2012 03:43 PM, Bengt Rutisson wrote:
>>>>> 
>>>>> Hi all,
>>>>> 
>>>>> I'd like some review for this change:
>>>>> http://cr.openjdk.java.net/~brutisso/7160728/webrev.00/
>>>>> 
>>>>> It introduces an extra logging level for G1. This is needed to do efficient performance analysis on G1 log files. Most of this will later on be replaced by a more general framework for the whole JVM.
>>>>> 
>>>>> This is part of the work for this CR:
>>>>> 7132559: G1: enhance / cleanup the PrintGCDetails output
>>>>> 
>>>>> Thanks,
>>>>> Bengt
>>> 
>>