RFR(M): 7114678: G1: various small fixes, code cleanup, and refactoring

[Bengt Rutisson]

Hi John,

Comments inline, but the summary is that I think this looks fine. Ship 
it! :-)

On 2012-07-17 19:53, John Cuthbertson wrote:
> Hi Bengt,
>
> Thanks for looking at the code. Replies inline...
>
> On 07/16/12 06:31, Bengt Rutisson wrote:
>>
>> Hi John,
>>
>> This looks good to me. Thanks for preparing the change.
>>
>> A couple of comments:
>>
>> collectionSetChooser.hpp
>>
>> The new class CSetChooserParUpdater is only instantiated in one place 
>> (in the ParKnownGarbageHRClosure constructor). There it always get 
>> true for the parallel value. Maybe we can remove the !parallel case 
>> from CSetChooserParUpdater?
>>
>
> Even though the _parallel attribute is always set to true in this 
> patch, it can have either true or false in the patch that builds on 
> this (when the class is instantiated from parallel and non-parallel 
> instances of the KnownGarbageTask). It's not a big deal to remove it 
> and refactor the two patches so that is in included in the second 
> patch but it will be being used almost immediately.

I see. No need to change back and forth. If the non-parallel case will 
come in use very soon we can keep it.

>> g1CollectedHeap.hpp
>>
>> A matter of taste. But I think it is difficult to follow the code 
>> when we have overloaded version of all of these methods: 
>> is_obj_dead(), is_obj_ill() and is_obj_dead_cond(). Would it be 
>> cleaner to have is_obj_dead_cond(const oop obj, const HeapRegion* hr, 
>> const VerifyOption vo) accept NULL as hr and let the methods 
>> is_obj_dead() and is_obj_ill() handle that they get hr==NULL ? That 
>> would mean that we would not need any of these methods to be 
>> overloaded to take hr or not.
>>
>
> I originally asked Tony the same question. :) The overloading saves an 
> extra unnecessary check of the heap region against null. Most of the 
> time when we call these methods we know the heap region and we know 
> it's not null.

Right, I just didn't think the extra null check would have much of a 
performance impact. But let's keep it as it is.

>
>>
>> heapRegion.hpp
>>
>> How about replacing the HR_FORMAT and HR_FORMAT_PARAMS macros with a 
>> HeapRegion::to_string() method?
>
> I'm not sure a "to_string" method is the right approach here. It 
> suggests that it passes a string back to it's caller. Which implies 
> that someone is responsible for allocating and deallocating the buffer 
> returned by to_string. A better approach might be a print_extended() 
> method which does not require any external storage.
>
> Since these macros are uses extensively throughout the code - can I do 
> this as a separate change?

That's fine. This is not directly related to the other changes so we can 
come back to this at some later point.

Thanks for the clarifications!
Bengt

>
> Thanks again for the review.
>
> JohnC
>