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