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

[Bengt Rutisson]

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

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

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

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
>