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

[Bengt Rutisson]

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