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

[John Cuthbertson]

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

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

Thanks again for the review.

JohnC