RFR(S): 8007772: G1: assert(!hr->isHumongous() || mr.start() == hr->bottom()) failed: the start of HeapRegion and MemRegion should be consistent for humongous regions

John Cuthbertson john.cuthbertson at oracle.com
Mon Feb 11 20:33:15 UTC 2013


Hi Bengt,

Jesper's code does that by the having the "start_of_humongous" clause 
before the iterate call in the condition. The compiler should 
short-circuit that and skip the iterate call for an h-obj.

My concern/objection is that is not explicitly clear from the code and 
you're comment sort of confirms that. :)

You have to really study the code to satisfy yourself that it's correct.

Thanks,

JohnC

On 2/11/2013 12:17 PM, Bengt Rutisson wrote:
>
> Jesper,
>
> I'll let John correct me if I'm wrong here, but I think the idea 
> behind the original patch was to avoid the call to 
> _nextMarkBitMap->iterate(), so I think we miss that enhancement if we 
> include it in the first if-statement.
>
> I agree that it would be nice to get rid of the code duplication, but 
> I am not quite sure how.
>
> Bengt
>
>
> On 2/11/13 7:48 PM, Jesper Wilhelmsson wrote:
>> John,
>>
>> Would it make sense to replace this code
>>
>> if (mr.is_empty()) {
>>   giveup_current_region();
>>   regular_clock_call();
>> } else if (_curr_region->isHumongous() && mr.start() == 
>> _curr_region->bottom()) {
>>   if (_nextMarkBitMap->isMarked(mr.start())) {
>>     // The object is marked - apply the closure
>>     BitMap::idx_t offset = 
>> _nextMarkBitMap->heapWordToOffset(mr.start());
>>     bitmap_closure.do_bit(offset);
>>   }
>>   // Even if this task aborted while scanning the humongous object
>>   // we can (and should) give up the current region.
>>   giveup_current_region();
>>   regular_clock_call();
>> } else if (_nextMarkBitMap->iterate(&bitmap_closure, mr)) {
>>   giveup_current_region();
>>   regular_clock_call();
>> } else {
>>
>>
>> with this code
>>
>>
>> bool start_of_humongous = _curr_region->isHumongous() &&
>>                           mr.start() == _curr_region->bottom();
>>
>> if (mr.is_empty() || start_of_humongous ||
>>     _nextMarkBitMap->iterate(&bitmap_closure, mr)) {
>>
>>   if (start_of_humongous && _nextMarkBitMap->isMarked(mr.start())) {
>>     // The object is marked - apply the closure
>>     BitMap::idx_t offset = 
>> _nextMarkBitMap->heapWordToOffset(mr.start());
>>     bitmap_closure.do_bit(offset);
>>   }
>>   // Even if this task aborted while scanning the humongous object
>>   // we can (and should) give up the current region.
>>   giveup_current_region();
>>   regular_clock_call();
>> } else {
>>
>>
>> to avoid some code duplication?
>> /Jesper
>>
>>
>> On 11/2/13 7:01 PM, John Cuthbertson wrote:
>>> Hi Everyone,
>>>
>>> Can I get a couple of reviews of this fix - the webrev can be found at:
>>> http://cr.openjdk.java.net/~johnc/8007772/webrev.0/
>>>
>>> Summary:
>>> An assert I asked Tao to add during his recent scanning optimization 
>>> for
>>> Humongous regions was failing. The reason was because the local 
>>> marking task
>>> could have been signaled to abort (because it exceeded it's time 
>>> interval) while
>>> scanning the humongous object. In this case we would restart the 
>>> marking step
>>> with a memory region with a slightly different starting address 
>>> causing the
>>> assert to trip. Without the assert this would nullify Tao change and 
>>> cause
>>> further unnecessary scanning of (a subset of) the humongous object. 
>>> We should be
>>> giving up the current humongous region even if the task is asked to 
>>> abort.
>>>
>>> Testing:
>>> The failing tests from nsk.stress and vm.gc nightly tests.
>>>
>>> Without fix:
>>> TOTAL TESTS IN RUN: 154
>>> TEST PASS: 102; 66% rate
>>> TEST FAIL: 52; 33% rate
>>> TEST UNDEFINED: 0; 0% rate
>>> TEST INCOMPLETE: 0; 0% rate
>>> TESTS NOT RUN: 0
>>>
>>> With fix:
>>> TOTAL TESTS IN RUN: 154
>>> TEST PASS: 153; 99% rate
>>> TEST FAIL: 1; 0% rate
>>> TEST UNDEFINED: 0; 0% rate
>>> TEST INCOMPLETE: 0; 0% rate
>>> TESTS NOT RUN: 0
>>>
>>> The remaining failure is FinalizerTest03 - a known failure.
>>>
>>> Thanks,
>>>
>>> JohnC
>>>
>




More information about the hotspot-gc-dev mailing list