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

Bengt Rutisson bengt.rutisson at oracle.com
Wed Oct 19 02:45:53 PDT 2011


Hi again,

Updated webrev based on comments from Tony:

http://cr.openjdk.java.net/~brutisso/7097516/webrev.02/

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