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

[Jesper Wilhelmsson]

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
>