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

[Jon Masamitsu]

Bengt,

I looked at the

http://cr.openjdk.java.net/~brutisso/8040722/webrev.00-01.diff/
http://cr.openjdk.java.net/~brutisso/8040722/webrev01-02.diff/
http://cr.openjdk.java.net/~brutisso/8040722/webrev.02-03-diff/

All changes look good.  Thanks.

Reviewed.

Jon

On 4/17/2014 5:45 AM, Bengt Rutisson wrote:
>
> Hi again,
>
> Thomas helped me track down a race that means that 
> G1UpdateRSOrPushRefOopClosure::do_oop_nv() can not assert that the 
> from and to regions are different. Reverted back to using an 
> if-statement rather than an assert, just like the original code.
>
> New webrev:
> http://cr.openjdk.java.net/~brutisso/8040722/webrev.03/
>
> Incremental webrev:
> http://cr.openjdk.java.net/~brutisso/8040722/webrev.02-03-diff/
>
> Thanks,
> Bengt
>
> On 2014-04-17 13:27, Bengt Rutisson wrote:
>>
>> 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
>>>
>>>
>>
>