[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-gc-dev
Subject:    Re: RFR (M): JDK-8040722: G1: Clean up usages of heap_region_containing
From:       Bengt Rutisson <bengt.rutisson () oracle ! com>
Date:       2014-04-17 16:48:38
Message-ID: 535005E6.9010606 () oracle ! com
[Download RAW message or body]


Thanks for the reviews Jon and Thomas!

Bengt

On 4/17/14 5:13 PM, Jon Masamitsu wrote:
> 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
>>>>
>>>>
>>>
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]