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

[Vitaly Davidovich]

Hi John,

Thanks for the feedback.  Regarding the asserts question, here's an example
g1HotCardCache.cpp):

assert(worker_i < (int) (ParallelGCThreads == 0 ? 1 : ParallelGCThreads),
"incorrect worker id");

Would be useful to see worker_i here?

Another in g1CardCounts.hpp:

  67   void check_card_num(size_t card_num, const char* msg) {
  68     assert(card_num >= 0 && card_num < _committed_max_card_num, msg);
  69   }

The msg will have the card_num, but you won't have _committed_max_card_num
reported, for example.  That's the kind of thing I meant.

There may be a few more examples like that, but this came up during quick
scan.

Thanks

Sent from my phone
On Jan 23, 2013 6:36 PM, "John Cuthbertson" <john.cuthbertson at oracle.com>
wrote:

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130123/ce8f37a1/attachment.htm>