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

Vladimir Kozlov vladimir.kozlov at
Mon Oct 31 17:40:52 UTC 2011

I agree with Tony, don't rely on print() definition in AllocatedObj since it is 
only defined in non product VM (because in product version AllocatedObj should 
not have virtual table and have size 0). Consider AllocatedObj::print() as 
backup for classes which does not define they own print() methods when you 
debugging VM.


Tony Printezis wrote:
> Bengt,
>> 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.
> I agree. And I'm really not sure why the print methods were introduced 
> on AllocatedObj. Having said that, I think it's helpful to define:
> virtual void print() const {
>     print_on(tty);
>   }
> on CollectedHeap to show explicitly what it does. If I remove it I feel 
> as if it might confuse the reader who'd never think of looking in the 
> AllocatedObj class for the print definition (I know I wouldn't....). 
> Also, if someone does the obvious thing and removes the print methods 
> from AllocatedObj :-) they'd have to re-introduce it on CollectedHeap. 
> I'm inclined to leave it in to keep CollectedHeap independent of 
> AllocatedObj (at least in that respect).
>> Also, a nit pick, but the whitespace changes at lines 414 and 417 in 
>> universe.hpp seem kind of strange to me.
> The idea was to get better alignment but I think I added extra 
> whitespace in line 414 for some reason. Does this look better?
>  414   static bool verify_in_progress() { return _verify_in_progress; }
>  415   static void verify(bool allow_dirty = true, bool silent = false,
>  416                      VerifyOption option = VerifyOption_Default );
>  417   static int  verify_count()       { return _verify_count; }
> Or you'd prefer:
> static bool verify_in_progress() {
>   return _verify_in_progress;
> }
> static void verify(bool allow_dirty = true, bool silent = false,
>                    VerifyOption option = VerifyOption_Default );
> static int verify_count() {
>   return _verify_count;
> }
>> 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.
> I could add a max of how many per-region entries we'll write to the 
> hs_err file and basically skip the rest (by adding an appropriate 
> warning in the log of course). Having said that, this might make this 
> output useless given that the entries we'll skip might be the ones that 
> we'd need. So, I'm inclined not to do that.
>> 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. 
> Agreed.
>> One alternative would of course be to log this in a separate hs_err_g1 
>> file...
> I really don't think we need to go there! We'll generate the same amount 
> of output but now split over two files! And the customers will have to 
> remember to also keep around and send us the hs_err_g1 files too. :-)
> Tony
>> 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:
>>> 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

More information about the hotspot-gc-dev mailing list