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

[David Lindholm]

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