Request for review (S): 7097516 G1: assert(0<= from_card && from_card<HeapRegion::CardsPerRegion) failed: Must be in range.

Stefan Karlsson stefan.karlsson at oracle.com
Fri Oct 21 04:12:05 PDT 2011


On 10/21/2011 12:47 PM, Tony Printezis wrote:
>
>
> On 10/20/2011 04:31 PM, Stefan Karlsson wrote:
>> I understand how reusing capacity(), used(), etc. could have made 
>> parts of the G1 code easier to write. Is it worth it, though?
>
> ...compared to what?

Compared to breaking the abstraction of a space and having this 
unintuitive code that was the cause of this bug?

> Always checking whether a region is humongous or not? Absolutely.

I looked for places where we used the capacity() function, and found 
only places where we do explicit checks for humongous regions.

> This is something that's worked out nicely in G1 so far.

Could you be more specific? I'm just asking since we had a bug because 
of this implementation.

> The only aspect of this I'd change would be for the region iterators 
> to skip continues humongous regions, given that we filter them out 
> most of the time anyway.
>
>> Could you point me to some of the usages in G1 where we rely on 
>> capacity(), used(), etc. to report the "extended" range of Start 
>> Humonguos regions?
>
> Look at when we add a region to one of the heap region sets. We do 
> check whether it's humongous or not but that's to calculate the number 
> of regions it spans. In fact, maybe we'd be better off just 
> calculating that as capacity() / RegionSize and skip the test. In 
> fact, this is a nice example of how this approach simplifies the code.

Are you talking about, for example: 
HeapRegionSetBase::calculate_region_num, and that all call paths to it 
does something like:
   if (!hr->isHumongous()) {
     _calc_region_num         += 1;
   } else {
     _calc_region_num         += calculate_region_num(hr);
   }

I don't agree that that would simplify the code. Yes, maybe shrink the 
number of lines but not lower the complexity, IMHO.

StefanK

>
> An additional point: when we set up the block offset table for a 
> humongous region, all the entries on it have to point to the bottom of 
> the starts humongous region (as the rest do not have an object start), 
> so it's again natural for that whole area to be covered by that region 
> wrt to used and capacity.
>
> Tony
>
>> thanks,
>> StefanK
>>
>>>
>>> Note, however, that the only time we really need to "original" 
>>> version of the above methods is in the current RSet code given that 
>>> each RSet "chunk" corresponds to exactly one region. Everything else 
>>> in the code relies on the fact that the above methods on SH regions 
>>> are the "extended" versions. So, we save ourselves a lot of trouble 
>>> but not having to check whether a region is SH or not before 
>>> querying it.
>>>
>>> I'd also like to point out that I'd like us to re-architect the 
>>> RSets so that they do not rely on regions but, instead, on (fixed 
>>> size) memory ranges (could be equal to the region size, could be 
>>> smaller, could be larger). That way, we'll eliminate the need for 
>>> the "raw" version of is_in_reserved() and deal with SH and 
>>> non-humongous regions uniformly.
>>>
>>> Tony
>>>
>>> On 10/20/2011 04:21 AM, Stefan Karlsson wrote:
>>>> Hi Bengt,
>>>>
>>>> On 10/19/2011 11:45 AM, Bengt Rutisson wrote:
>>>>>
>>>>> Hi again,
>>>>>
>>>>> Updated webrev based on comments from Tony:
>>>>>
>>>>> http://cr.openjdk.java.net/~brutisso/7097516/webrev.02/
>>>>
>>>> Looks good to me.
>>>>
>>>> But, I find it really odd that Space::is_in_reserved(...) looks at 
>>>> the expanded size when working on Start Continues regions, and 
>>>> HeapRegion::is_in_reserved_raw looks at the "original" region size. 
>>>> To me, it would be more natural if Space::is_in_reserved(...) 
>>>> actually did what one would expect, look at the original heap 
>>>> region size, and then have a somthing like 
>>>> HeapRegion::is_in_reserved_expanded to expand the size for Start 
>>>> Continues regions.
>>>>
>>>> StefanK
>>>>
>>>>>
>>>>> Bengt
>>>>>
>>>>> On 2011-10-19 09:14, Bengt Rutisson wrote:
>>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> Could I have a couple of reviews of this change?
>>>>>>
>>>>>> http://cr.openjdk.java.net/~brutisso/7097516/webrev.01/
>>>>>>
>>>>>> Some background:
>>>>>>
>>>>>> The humongous start regions in G1 gets their _end marker set to 
>>>>>> the end of the last humongous continues region for the humongous 
>>>>>> object. This means that the start region overlaps all of the 
>>>>>> continues regions. Code that handles regions need to be aware of 
>>>>>> this special case.
>>>>>>
>>>>>> However, PerRegionTable::add_reference_work() had a subtle issue 
>>>>>> with this special case. Here is what happened:
>>>>>>
>>>>>> A thread, TA, looks up the PerRegionTable, PRT,  for a particular 
>>>>>> heap region. This heap region happens to be a humongous continues 
>>>>>> region. Let's call it HRA.
>>>>>>
>>>>>> Another thread, TB, wants to look up the PerRegionTable for the 
>>>>>> humongous start region of that same humongous object. Let's call 
>>>>>> that heap region HRB. If we are unlucky TB is the first thread 
>>>>>> that wants the PerRegionTable for HRB and thus has to allocate 
>>>>>> it. Now, there is a limit to how many PerRegionTables we allow in 
>>>>>> the system. If this limit has been reached TB will find a 
>>>>>> suitable existing PerRegionTable and coarsen that to use a bitmap 
>>>>>> for the remset instead of the PerRegionTable.
>>>>>>
>>>>>> This means that TB can actually "steal" the PRT from underneath TA.
>>>>>>
>>>>>> TA is aware that this can happen. So, before it tries to do any 
>>>>>> work with the information in the PRT it tries to verify that it 
>>>>>> hasn't been "stolen" (actually coarsened). It does this by 
>>>>>> storing the heap region for the PRT in a local variable (called 
>>>>>> loc_hr) and then check that the reference that it is about to 
>>>>>> handle still belong to the heap region.
>>>>>>
>>>>>> Basically:
>>>>>>
>>>>>>     HeapRegion* loc_hr = hr();
>>>>>>     if (loc_hr->is_in_reserved(from)) {
>>>>>>        ...
>>>>>>     }
>>>>>>
>>>>>> Normally this is safe. If TA gets HRB in loc_hr the test will 
>>>>>> fail. However, in the case where HRB is the humongous start 
>>>>>> region the test will not fail. Instead we will pass the test and 
>>>>>> execute the code inside the if. There we try to calculate a card 
>>>>>> index based on the bottom of the heap region and the from 
>>>>>> address. Now this card index will be larger than the number of 
>>>>>> cards we have per region, since from actually is not in the region.
>>>>>>
>>>>>> The fix is to introduce a new method called 
>>>>>> HeapRegion::is_in_reserved_raw() and use this in the test. The 
>>>>>> _raw method will not return false even for humongous start 
>>>>>> objects in the example above.
>>>>>>
>>>>>> I picked the name is_in_reserved_raw() to be consistent with the 
>>>>>> heap_region_containing()/heap_region_containing_raw() methods. I 
>>>>>> am not particularly fond of the name, so if there are better 
>>>>>> suggestions I am happy to change it.
>>>>>>
>>>>>> Testing
>>>>>> I was able to reproduce the issue on bur398-216 after 40-50 
>>>>>> iterations. With my fix it has now ran 540 iteration without 
>>>>>> failing.
>>>>>>
>>>>>> Other uses of is_in_reserved()
>>>>>> Tony and I went through the G1 code and looked for other uses of 
>>>>>> HeapRegion::is_in_reserved(). As far as we can tell the other 
>>>>>> uses all avoid the issue that 
>>>>>> PerRegionTable::add_reference_work() has. Most of them because 
>>>>>> they are used during evacuation where humongous objects are not 
>>>>>> present at all.
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~brutisso/7097516/webrev.01/
>>>>>>
>>>>>> CR:
>>>>>> 7097516 G1: assert(0<= from_card && 
>>>>>> from_card<HeapRegion::CardsPerRegion) failed: Must be in range.
>>>>>> http://monaco.us.oracle.com/detail.jsf?cr=7097516
>>>>>>
>>>>>> Thanks,
>>>>>> Bengt
>>>>>
>>>>
>>



More information about the hotspot-gc-dev mailing list