Code review request: 6855834: G1: minimize the output when -XX:+PrintHeapAtGC is set (S)

[John Coomes]

Tony Printezis (Antonios.Printezis at Sun.COM) wrote:
> John,
> 
> Thanks, see inline.
> 
> John Coomes wrote:
> > Tony Printezis (Antonios.Printezis at Sun.COM) wrote:
> >   
> >> One more spin:
> >>
> >> http://cr.openjdk.java.net/~tonyp/6855834/webrev.2/
> >>
> >> All the recent changes are in g1CollectedHeap.cpp, in the print_on() 
> >> method (which reflect the updated output that John Coomes suggested). 
> >> One more review and I'll push it...
> >>     
> >
> > Looks good ... except for some whitespace nits in g1CollectedHeap.cpp
> > (sorry):
> >   
> No need to apologize! :-)
> > 1.  if (PrintHeapAtGC){    <---- no space before the '{' 
> >
> > 	This occurs 4 times (I think).
> >   
> Yep, again bad cut-and-paste.

Sure, I guessed as much.

> > 2.  No blank line between methods print(), print_on(outputStream*) and
> > print_on(outputStream*, bool) (lines 2336, 2339).
> >   
> I'll add them, but it's the same in genCollectedHeap.cpp:
> 
> void GenCollectedHeap::print() const { print_on(tty); }
> void GenCollectedHeap::print_on(outputStream* st) const {
>   for (int i = 0; i < _n_gens; i++) {
>     _gens[i]->print_on(st);
>   }
>   perm_gen()->print_on(st);
> }

Thanks.  I think it's easier to read with the blank line.

> > 3.  No space between the double quote and INTPTR_FORMAT or
> > SIZE_FORMAT in the new print_on() code.  AFAICT, most of hotspot uses
> > this:
> >
> > 	print("count = " SIZE_FORMAT "K", count);
> >
> > instead of
> >
> > 	print("count = "SIZE_FORMAT"K", count);
> >
> > I find the latter harder to parse.  I see some existing examples of
> > the latter in the same file, but haven't seen it outside of G1.
> >   
> I personally find the second version easier to parse, given that it's a 
> bit clearer where there's white space and where there's not. I'll change 
> it in an attempt to be consistent though.

Strange how we see things.  The "SIZE_FORMAT" in the latter is what I
notice at first glance, which makes me think the wrong thing is
quoted.

-John

> >> Tony Printezis wrote:
> >>     
> >>> Thanks to Ramki for the speedy code review. I applied a couple of 
> >>> minor changes, here's the second attempt:
> >>>
> >>> http://cr.openjdk.java.net/~tonyp/6855834/webrev.1/
> >>>
> >>> Tony
> >>>
> >>> Tony Printezis wrote:
> >>>       
> >>>> http://cr.openjdk.java.net/~tonyp/6855834/webrev.0/
> >>>>
> >>>> The CR has more information on the new format:
> >>>>
> >>>> Tony
> >>>>
> >>>>         
> >> -- 
> >> ---------------------------------------------------------------------
> >> | Tony Printezis, Staff Engineer   | Sun Microsystems Inc.          |
> >> |                                  | MS UBUR02-311                  |
> >> | e-mail: tony.printezis at sun.com   | 35 Network Drive               |
> >> | office: +1 781 442 0998 (x20998) | Burlington, MA 01803-2756, USA |
> >> ---------------------------------------------------------------------
> >> e-mail client: Thunderbird (Linux)
> >>
> >>
> >>     
> >
> >   
> 
> -- 
> ----------------------------------------------------------------------
> | Tony Printezis, Staff Engineer    | Sun Microsystems Inc.          |
> |                                   | MS BUR02-311                   |
> | e-mail: tony.printezis at sun.com    | 35 Network Drive               |
> | office: +1 781 442 0998 (x20998)  | Burlington, MA01803-0902, USA  |
> ----------------------------------------------------------------------
> e-mail client: Thunderbird (Solaris)
> 
>