CRR (S): 7099849: G1: include heap region information in hs_err files

[Bengt Rutisson]

Hi Tony,

Thanks for fixing this! I think it will be great to have this 
information in the hs_err file.

It is also good that you clean up the print() and print_on() structure. 
But I think you can take it one step further.

In collectedHeap.hpp you now have:

virtual void print() const {
     print_on(tty);
   }

But CollectedHeap inherits AllocatedObj (through CHeapObj). And 
AllocatedObj defines print() as:

void AllocatedObj::print() const       { print_on(tty); }

So, I think you can just remove the print() method in CollectedHeap. 
This will also get rid of the shadowing of AllocatedObj::print() by 
CollectedHeap::print(). That looks kind of fishy to me.


Also, a nit pick, but the whitespace changes at lines 414 and 417 in 
universe.hpp seem kind of strange to me.

Finally, just for the record, we have tests that run without setting 
-Xms or -Xmx. If such a test ends up on a large machine we can end up 
with many more than 1000-2000 regions. I have seen ten times as many 
regions.

It might be unlikely that customers run without limiting the heap size 
on this large machines, but we will run into hs_err files with 10s of 
thousands of heap regions in our own testing. Personally I think the 
information is useful enough for this to be acceptable. One alternative 
would of course be to log this in a separate hs_err_g1 file...

Bengt


On 2011-10-12 18:49, Tony Printezis wrote:
> Hi all,
>
> (I'm also copying the runtime alias, as this change will concern them 
> too)
>
> I'd like to get a couple of reviews for this change:
>
> http://cr.openjdk.java.net/~tonyp/7099849/webrev.0/
>
> Some background: when trying to track down issues in G1 it is often 
> very helpful to know what type of regions the heap has and/or get 
> information on a particular region (whether it's humongous, whether 
> it's young, how full it is, etc.). We thought it'd be a good idea to 
> include the per-region information in the hs_err file to always have 
> it available after a crash.
>
> I don't think the changes in the webrev are too controversial. The 
> reason I wanted to give a heads up to both groups was to point out 
> that this change will increase the size of the hs_err files when G1 is 
> used. It's common to have 1,000 to 2,000 regions in the heap, if 
> larger heaps are used (much fewer if smaller heaps are used), which 
> means that the hs_err file will have this many extra lines. Does 
> anyone have any concerns about this?
>
> I attached an example hs_err file obtained from a workspace with this 
> change applied.
>
> BTW, I also cleaned up a bit the way the print() methods on the heap 
> are defined (I pushed the default behavior to the superclass, where 
> possible).
>
> Tony
>