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

[Tony Printezis]

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?

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

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.

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?

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?

g1Log.cpp

Is it worth turning G1LogLevel into lowercase to make the comparison 
case-insensitive?

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