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

[Tao Mao]

Looks good. I think, if one reads the comments above, one would be easy 
to follow the John's code.

Tao

On 2/11/2013 12:51 PM, Jesper Wilhelmsson wrote:
> 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