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

[Tony Printezis]

John,

Thanks for the review! Yes, I will make the change you're recommending; 
it's an excellent suggestion.

Tony

john cuthbertson - Sun Microsystems wrote:
> 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
>>>>
>>
>