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

[Bengt Rutisson]

Hi John,

On 2/11/13 9:33 PM, John Cuthbertson wrote:
> 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.

Yes, you and Jesper are right. My bad.

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

:) Now I can't really argue with that ;)

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