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]

On 11/2/13 9:28 PM, John Cuthbertson wrote:
> Hi Jesper,
>
> On 2/11/2013 10:48 AM, Jesper Wilhelmsson wrote:
>> John,
>>
>> Would it make sense to replace this code
> [snip]
>>
>> 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 {
>>
>
> Sure, semantically they're the same but it it any clearer?  You have to study
> the code and remember short-circuiting to realize that, if the h-obj was marked,
> then we would not be applying the bitmap closure twice (one in the iterate call
> and once directly), and (if the object is not marked) that there's no redundant
> scanning of the bitmap.

I guess it's a matter of taste, to me it is a lot clearer. But sure, it takes a 
second look at the code to see that short-circuiting matters here.

The version with code duplication is a bit confusing to me and requires a second 
look to see that there is no difference between the different clauses in the 
if/else if.

I would argue that for someone who is new to the code, the non-duplicated code 
would be easier to get familiar with due to (at a first glance) less complicated 
code. And you don't have to know that the short-circuiting is necessary until 
you know the code well enough to realize that it is necessary, if you see what I 
mean.

We could also add a comment :-)
/Jesper


>
> JohnC