RFR(M/L): 7176479: G1: JVM crashes on T5-8 system with 1.5 TB heap

[John Cuthbertson]

Hi Vitaly,

Second response. Details inline....

On 1/15/2013 8:49 PM, Vitaly Davidovich wrote:
>
> A few more comments/suggestions:
>
> In g1CardCounts::ptr_2_card_num(), I'd assert that card_ptr >= 
> _ct_bot.  This is mostly to avoid a null card_ptr (or some other bogus 
> value) causing the subtraction to go negative but then wrap around to 
> a large size_t value that just happens to fit into the card range.  
> Unlikely and maybe this is too paranoid, so up to you.
>

Good idea. Done. I'm all for being paranoid. I've also changed the 
subtraction to use pointer_delta:

     assert((size_t)card_ptr >= (size_t)_ct_bot, "wraparound?");
     size_t card_num = pointer_delta(card_ptr, _ct_bot, sizeof(jbyte));


> Also in this class, it's a bit strange that G1CardCounts::is_hot() 
> also increments the count.  I know the comments in the header say that 
> count is updated but the name of the method implies it's just a read.  
> Maybe call add_card_count() and then add an is_hot(int) method and 
> call that with the return value of add_card_count?
>

Let me think about that one. I was looking for a much simpler interface 
that returned whether a card was was hot. How it made that determination 
was up to it. But I'm leaning toward following your suggestion.

> G1CardCounts::clear_region -- there are some casts of const jbyte to 
> jbyte at bottom of method.  Perhaps if ptr_2_card_num() were changed 
> to be taking const jbyte you wouldn't need the casts.  I think marking 
> as much things const as possible is good in general anyway ...
>

Good point. Done.

> In the various places where values are asserted to be in some range, 
> it may be useful to add the valid range to the error message so that 
> if it triggers you get a bit more context/diagnostic info.
>

I thought I was doing that in most cases. Can you give some examples?

Thanks,

JohnC