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

[Jon Masamitsu]

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

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.

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

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.

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