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]

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