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

[Bengt Rutisson]

Hi again,

Here's an updated webrev:
http://cr.openjdk.java.net/~brutisso/8040722/webrev.01/

And here's the incremental change compared to the previous one:
http://cr.openjdk.java.net/~brutisso/8040722/webrev.00-01.diff/

Thanks,
Bengt


On 2014-04-17 01:15, Jon Masamitsu wrote:
>
> 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
>>>
>>
>