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

[Bengt Rutisson]

Webrev and scp are messing with me.

Here is the final webrev:
http://cr.openjdk.java.net/~brutisso/7097516/webrev.04/

(Just comment changes compared to the previous one.)

Bengt

On 2011-10-20 10:33, Bengt Rutisson wrote:
>
> Hi Stefan,
>
> Thanks for you review!
>
> On 2011-10-20 10:21, 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.
>
> Here is the final webrev:
> http://cr.openjdk.java.net/~brutisso/7097516/webrev.03/
>
> Thanks Tony and Stefan. I am all set to push this now.
>
>> 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.
>
> I agree with you. This is actually a conceptual change. We would need 
> to make the start humongous regions keep their size and then just add 
> extra information to them regarding the end of the last continues 
> humongous region. This could maybe be stored as a Space in the start 
> region.
>
> We need to make sure that is_in_reserved() is consistent with other 
> methods such as capacity() and containing(). To me it is more natural 
> to explicitly state that you are interested in the special case of 
> start humongous regions than to have to explicitly state that you are 
> not interested in this special case. Also if we would do this change 
> we would never have overlapping regions and we would not have to 
> filter out continues regions in places where we have to do that today.
>
> I discussed this with Tony a couple of days ago, so I know he 
> disagrees. ;-) I'll let him state his argument.
>
> Anyway, such a change should be handled in a CR of its own. I'll go 
> ahead and push my small change now.
>
> Thanks,
> Bengt
>
>>
>> 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
>>>
>>
>