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

[David Lindholm]

Tom,

Thanks for your comments!

On 2015-11-04 22:49, Tom Benson wrote:
> 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)

I don't think I can, since ext_marked_bytes will be the objectsize of 
the humongous object. But instead I
added this check:

(num_regions-1) * HeapRegion::GrainBytes <  exp_marked_bytes <= 
num_regions * 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.

Yes, your wording is better. I changed to that.

> 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...

Yes, you are correct. Changed to next_region_in_humongous instead.

Tom and Thomas, could you both please look at the latest webrev:

http://cr.openjdk.java.net/~david/JDK-8139867/webrev.04/
http://cr.openjdk.java.net/~david/JDK-8139867/webrev.03-04/ (diff)


Thanks,
David

> 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
>