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

[Bengt Rutisson]

Hi Thomas,

Thanks for looking at this!

Details inlined below.

Updated webrev:
http://cr.openjdk.java.net/~brutisso/8040722/webrev02/

Diff against the 01 version:
http://cr.openjdk.java.net/~brutisso/8040722/webrev01-02.diff/

Thanks,
Bengt

On 2014-04-17 12:51, Thomas Schatzl wrote:
> Hi Bengt,
>
> On Thu, 2014-04-17 at 10:26 +0200, Bengt Rutisson wrote:
>> 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/
>>
>    a few comments, none of them are critical:
>
>   - g1CollectedHeap.inline.hpp, in
> G1CollectedHeap::heap_region_containing_raw:
>
> The != NULL check is subsumed by the other. Is it possible to merge them
> with a useful error message?
>
> E.g. assert(_g1_reserved.contains(...), err_msg("Address "PTR_FORMAT" is
> outside of the heap ranging from ["PTR_FORMAT" to "PTR_FORMAT")", addr,
> _g1_reserved.start(), _g1_reserved.end());
>
> I would prefer to have one assert instead of separating them.

I added an err_msg() to the second assert to log the value of the 
failing address. But I keep the explicit NULL check since I think this 
cleanup relies heavily on this invariant.

>
>   - in CMTask::setup_for_region(): maybe the error message for the !=
> NULL assert could be improved.

Changed to "claim_region() should have filtered out NULL regions".

>
>   - G1CollectedHeap::heap_region_containing_raw documentation: maybe
> better:
>
> // Returns the HeapRegion the given address addr contains. Addr must not
> be NULL.
>
>   - G1CollectedHeap::heap_region_containing() documentation: I would
> prefer something like:
>
> // Returns the HeapRegion the given address addr contains. Addr must not
> be NULL. If addr is within a humongous continues region, it returns its
> humongous start region.
>
> Imo, to name a region, using "<type-name> region" (e.g. humongous
> continues region) is better than trying to invent new descriptions like
> continuing humonguous region.

Sounds good. I updated the comments to be:


   // Returns the HeapRegion that contains addr. addr must not be NULL.
   template <class T>
   inline HeapRegion* heap_region_containing_raw(const T addr) const;

   // Returns the HeapRegion that contains addr. addr must not be NULL.
   // If addr is within a humongous continues region, it returns its 
humongous start region.
   template <class T>
   inline HeapRegion* heap_region_containing(const T addr) const;

Thanks again for looking at this!

Bengt


>
> Thanks,
> Thomas
>
>