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

[Bengt Rutisson]

On 2011-10-31 18:40, Vladimir Kozlov wrote:
> 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). 

I missed the fact that the AllocatedObj::print() is only defined in 
debug builds. That of course makes it impossible for CollectedHeap to 
rely on it.

> Consider AllocatedObj::print() as backup for classes which does not 
> define they own print() methods when you debugging VM.

I see. But why is it in that case not declared virtual?

Bengt

>
> Regards,
> Vladimir
>
> 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:
>>>>
>>>> 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
>>>>
>>>