Code Review Request: 6819085 G1: use larger and/or user settable region size

[john cuthbertson - Sun Microsystems]

Hi Tony,

The changes look good to me. I do have one question: should we change 
the line
"const size_t cards_per_region = HeapRegion::GrainBytes >> 
CardTableModRefBS::card_shift;" in g1CollectedHeap.cpp (it's around line 
1551) to use HeapRegion::CardsPerRegion for the sake of consistency?

Regards,

JohnC

On 08/05/09 09:28, Tony Printezis wrote:
> Jon,
>
> Thanks for the review! See inline.
>
> Jon Masamitsu wrote:
>> g1CollectedHeap.hpp
>>
>> Does this deserve a comment on what it is or
>> what it is used for?
>>
>> 173   static size_t _very_large_in_words;
>>
>> Ok, having read down a little further, how about
>> just changing the variable to _humungous_region_threshold.
> That's a good point. I tried to stay close to the existing name but I 
> agree, the one you propose is more appropriate. I actually changed it 
> to _humongous_object_threshold_in_words (yes, a bit verbose; but it's 
> only used in a handful of places and it's more descriptive).
>> arguments.cpp
>>
>> 1281 // Maximum region size; we don't go higher than that
>> 1282 #define MAX_REGION_SIZE  ( 32 * 1024 * 1024 )
>>
>> A comment on why there is a max would be nice.
> // Maximum region size; we don't go higher than that. There are a
> // couple of reasons for having an upper bound. We don't want
> // regions to get too large, otherwise cleanup's effectiveness would
> // decrease as there will be fewer opportunities to find totally empty
> // regions after marking.
>> heapRegion.hpp
>>
>> Just because set_heap_region_size() looks like an
>> accessor function, a comment that it is doing more
>> than just setting a field would be helpful.
>>
>>  306   static void set_heap_region_size(uintx heap_region_size_bytes,
>>  307                                    uintx heap_region_size_log);
> Done. I included a comment and I also enamed it to 
> setup_heap_region_size()
>
>  // It sets up the heap region size (GrainBytes / GrainWords), as
>  // well as other related fields that are based on the heap region
>  // size (LogOfHRGrainBytes / LogOfHRGrainWords /
>  // CardsPerRegion). All those fields are considered constant
>  // throughout the JVM's execution, therefore they should only be set
>  // up once during initialization time.
>  static void setup_heap_region_size(uintx heap_region_size_bytes,
>                                     uintx heap_region_size_log);
>
>
> The new webrev is here:
>
> http://cr.openjdk.java.net/~tonyp/6819085/webrev.1/
>
> Apart from the changes Jon suggested, I also added a couple of casts 
> to keep gcc happy.
>> Otherwise, looks good.
> Thanks!
>
> Tony
>> On 07/30/09 14:21, Tony Printezis wrote:
>>> http://cr.openjdk.java.net/~tonyp/6819085/webrev.0/
>>>
>>> The idea is to allow the user to set the region size with a 
>>> parameter (-XX:G1HeapRegionSize=N).
>>>
>>> Tony
>>>
>