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

[Bengt Rutisson]

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?


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.


heapRegion.hpp

How about replacing the HR_FORMAT and HR_FORMAT_PARAMS macros with a 
HeapRegion::to_string() method?


Otherwise it looks good to me. Nice cleanups!

Bengt


On 2012-07-06 19:48, John Cuthbertson wrote:
> Hi Everyone,
>
> This is the first in a series of patches that were completed by Tony 
> just before he left Oracle. There will be a few more in the coming few 
> weeks.
>
> This first patch is a pre-cursor to another patch that introduces 
> iterators for heap regions, and is exactly what it says in the 
> synopsis (and description in the bug database). Tony sent a version of 
> this patch earlier in the year; I reviewed it and Tony incorporated 
> the feedback into this version. So I'm looking for a couple of 
> volunteers to also review these changes. The webrev can be found at: 
> http://cr.openjdk.java.net/~johnc/7114678/webrev.0/
>
> Testing: GC Test Suite with and without some stress options, NSK tests 
> (regression, gc, jit, and runtime), jprt.
>
> Thanks,
>
> JohnC