RFR (M): JDK-8040722: G1: Clean up usages of heap_region_containing

[Jon Masamitsu]

On 4/16/2014 12:08 PM, Bengt Rutisson wrote:
>
> Hi Jon,
>
> Thanks for looking at this!
>
> On 4/16/14 6:06 PM, Jon Masamitsu wrote:
>> Bengt,
>>
>> http://cr.openjdk.java.net/~brutisso/8040722/webrev.00/src/share/vm/gc_implementation/g1/g1CollectedHeap.inline.hpp.frames.html 
>>
>>>    45 G1CollectedHeap::heap_region_containing_raw(const T addr) const {
>>>    46   assert(addr != NULL, "invariant");
>> heap_region_containing_raw() does an assertion check against
>> null and heap_region_containing() uses heap_region_containing_raw()
>> so is the null checking assertion at calls to 
>> heap_region_containing() useful?
>> For example here
>>
>> http://cr.openjdk.java.net/~brutisso/8040722/webrev.00/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp.frames.html 
>>
>>
>> 2953 Space* G1CollectedHeap::space_containing(const void* addr) const {
>> 2954   assert(addr != NULL, "invariant");
>> 2955   return heap_region_containing(addr);
>
> I'm fine with removing these asserts, I just figured that it would be 
> nice to catch any issues one frame earlier than in 
> heap_region_containing(). But provided that we get good stack traces I 
> guess it is fine to let the assert in heap_region_containing_raw() 
> catch the issues.
>
> Unless anybody else objects I'll remove those extra asserts.

Seems 1 less line of code in several places but I'm fine with either way.

>>
>> http://cr.openjdk.java.net/~brutisso/8040722/webrev.00/src/share/vm/gc_implementation/g1/heapRegionSeq.inline.hpp.frames.html 
>>
>>
>>>    32   assert(addr != NULL, "addr must be != NULL");
>>>    33   assert(addr < heap_end(),
>>>    34                 err_msg("addr: "PTR_FORMAT" end: "PTR_FORMAT, 
>>> addr, heap_end()));
>>>    35   assert(addr >= heap_bottom(),
>>>    36         err_msg("addr: "PTR_FORMAT" bottom: "PTR_FORMAT, addr, 
>>> heap_bottom()));
>>>    37
>>
>> Don't need assertion at 32.  Assertion at 35 should caught the addr 
>> !=NULL case or
>> heap_bottom() is NULL in which case addr == NULL is a legal address.
>
> Right. I was actually going back and forth on this assert anyway. I'll 
> remove it.

Thanks.

>
>>
>> heap_region_containing_raw() no longer does the check for 
>> continuousHumongous.
>> heap_region_containing_raw() is called from lots of places. For example,
>>
>>  130 inline void ConcurrentMark::count_region(MemRegion mr, uint 
>> worker_id) {
>>  131   HeapWord* addr = mr.start();
>>  132   HeapRegion* hr = _g1h->heap_region_containing_raw(addr);
>>  133   count_region(mr, hr, worker_id);
>>  134 }
>>
>> Is that OK?  Should it now call heap_region_containing()?
>
> I think you missed that heap_region_containing_raw() and 
> heap_region_containing() have switched places in 
> g1CollectedHeap.inline.hpp. I had to do that to get the inlining right 
> now when heap_region_containing() calls heap_region_containing_raw().
>
> So, heap_region_containing_raw() has not changed. Thus, all calls to 
> it should still be correct.

Ok.

>
>>
>> Maybe we can improve on the name heap_region_containing_raw()
>> (the "raw" conveying little information).
>>
>> Would
>>
>> heap_region_containing_raw -> 
>> heap_region_containing_ignore_continuousHumongous
>>
>> be an improvement.  A bit long, I'll admit.  I'm OK with
>> heap_region_containing_ICH if the declaration explained what
>> the ICH stood for.
>
> I agree that heap_region_containing_raw() is not a great name. 
> However, it does not ignore continues humongous regions. It is 
> heap_region_containing() that does that. So, I guess your naming 
> suggestion would be for heap_region_containing() to change to 
> heap_region_containing_ignore_continues_humongous(), right?
>
> With my patch for allocating objects into the last continues region of 
> a humongous object heap_region_containing() will also be able to 
> return continues regions occasionally. So, I'm not convinced about the 
> name.
>
> Would it be ok to leave the names as they are for now?

That's fine.  That will give us a chance to come up with an
even better name.

Reviewed.

Jon

>
> Thanks,
> Bengt
>>
>> Jon
>>
>> On 4/16/2014 6:46 AM, Bengt Rutisson wrote:
>>>
>>> Hi everyone,
>>>
>>> Could I please have a few reviews of this cleanup?
>>>
>>> http://cr.openjdk.java.net/~brutisso/8040722/webrev.00/
>>>
>>> Background:
>>>
>>> Before the perm gen was removed 
>>> G1CollectedHeap::heap_region_containing() could return NULL for 
>>> addresses in the perm gen. This means that a lot of code had to 
>>> check the result since it could potentially be NULL.
>>>
>>> Now the only reason to return NULL is if the address passed in NULL. 
>>> In many places it is possible to know that the address is not NULL 
>>> and in the other places it is just as simple to check the address 
>>> for NULL before calling G1CollectedHeap::heap_region_containing() 
>>> rather than checking for NULL after having called it.
>>>
>>> So, to simplify the code a bit we can assert that 
>>> G1CollectedHeap::heap_region_containing() never returns NULL and 
>>> then only check before we call it in the places where we would 
>>> otherwise have passed NULL to it.
>>>
>>> Thanks,
>>> Bengt
>>
>