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

Stefan Karlsson stefan.karlsson at
Fri Oct 21 11:12:05 UTC 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.


> 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:
>>>> 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?
>>>>>> 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:
>>>>>> CR:
>>>>>> 7097516 G1: assert(0<= from_card && 
>>>>>> from_card<HeapRegion::CardsPerRegion) failed: Must be in range.
>>>>>> Thanks,
>>>>>> Bengt

More information about the hotspot-gc-dev mailing list