RFR(M): 8139867: Change how startsHumongous and continuesHumongous regions work in G1.

[Tom Benson]

Hi David,
Thanks for updating that.   My only other comments are pretty trivial.

concurrentMark.cpp:

1438     // For start_humongous regions, the size of the whole object 
will be
1439     // in exp_marked_bytes, so this check does not apply in this case.
1440     if (exp_marked_bytes > act_marked_bytes && 
!hr->is_starts_humongous()) {
1441       failures += 1;
1442     }

Couldn't you still do the check if the humongous obj doesn't exceed this 
region?  IE, the check could be
  ... && (!hr->is_starts_humongous() || exp_marked_bytes <= 
HeapRegion::GrainBytes)


heapRegion.hpp:

   48 // ....    If the humongous
   49 // object is larger than a heap region, the following regions will
   50 // be of type ContinuesHumongous. In this case the top() and end()
   51 // of the StartHumongous region will point to the end of that region.
   52 // The same will be true for all ContinuesHumongous regions except
   53 // the last, which will have its' top() at the objects' top.

Wording suggestions:  I think "the end of that region" is a little 
ambiguous.  And since end() never changes (and you say that elsewhere), 
that can be left out.  Perhaps something like:

    ... In this case the top() of the StartHumongous region and all
    ContinuesHumongous regions except the last will point to their own end.
    For the last ContinuesHumongous region, top() will equal the 
object's top.


g1CollectedHeap.hpp/cpp and heapRegionManager.hpp/cpp:

Could next_humongous_region be called next_region_in_humongous (or 
perhaps, humongous_object_continuation) to clarify it isn't just the 
next humongous region in the heap, but a continuation of the current 
humongous object?  I know, it's hard to get too excited by that...

Thanks,
Tom


On 11/4/2015 10:30 AM, David Lindholm wrote:
> Tom,
>
> Thank you for looking at this!
>
> On 2015-11-04 15:35, Tom Benson wrote:
>> Hi,
>> I'm still working through the whole change, but have a 
>> comment/question about this:
>>
>> On 11/4/2015 7:01 AM, David Lindholm wrote:
>>>>>    - can we add an assert in HeapRegion::block_is_obj() that the 
>>>>> new case
>>>>> can only occur with humongous objects, and that p must be in the same
>>>>> humongous object?
>>>> Ok, fixed.
>>
>> The assertions imply it can handle an address *within* a humongous 
>> object, but I think it will only work for pointers to the object base:
>>
>>  119   if (!this->is_in(p)) {
>>  120     HeapRegion* hr = g1h->heap_region_containing(p);
>>  121 #ifdef ASSERT
>>  122     assert(hr->is_humongous(), "This case can only happen for 
>> humongous regions");
>>  123     oop obj = oop(hr->humongous_start_region()->bottom());
>>  124     assert((HeapWord*)obj <= p, "p must be in humongous object");
>>  125     assert(p <= (HeapWord*)obj + obj->size(), "p must be in 
>> humongous object");
>>  126 #endif
>>  127     return hr->block_is_obj(p);
>>  128   }
>>
>> I think if p != the base of the humongous_start_region(), we should 
>> probably just return false from block_is_obj. (Or assert, if you 
>> never expect an interior pointer).  Otherwise the recursive call 
>> might return true (if p < top()), and then HeapRegion::block_size 
>> will be unhappy when it tries to get the object size.   And the call 
>> to is_obj_dead(oop(p)...) will also be broken.
>> Am I missing something?   Tnx,
>> Tom
>
> You are correct. I think that both the current solution and your 
> proposal works. In the current solution, "this" will be a 
> continousHumongous region and hr will be the startsHumongous region. 
> On line 127 block_is_obj() will be called for the other heapRegion 
> (the startsHumongous), and it will respond correctly.
>
> However, I like your solution better. I have changed to this:
>
> if (!this->is_in(p)) {
>   assert(is_continues_humongous(), "This case can only happen for 
> humongous regions");
>   return (p == humongous_start_region()->bottom());
> }
>
>
> Thanks,
> David