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

[Tony Printezis]

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

-- 
----------------------------------------------------------------------
| Tony Printezis, Staff Engineer    | Sun Microsystems Inc.          |
|                                   | MS BUR02-311                   |
| e-mail: tony.printezis at sun.com    | 35 Network Drive               |
| office: +1 781 442 0998 (x20998)  | Burlington, MA01803-0902, USA  |
----------------------------------------------------------------------
e-mail client: Thunderbird (Solaris)