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

[Tony Printezis]

OK, one more try for this one!

http://cr.openjdk.java.net/~tonyp/6819085/webrev.3/

One of the changes was to make HeapRegion::GrainBytes an int instead of 
a size_t that I had originally declared it as. Lots of places in the 
code assumed it was an int (as it was a member of an enum before) and 
gcc and the Windows compiler complained. I chose not to fix all the 
places where it was used and declare it as an int instead to minimize 
the number of changes. I also re-organized it a bit based on more 
feedback from the group (moved the heap region calculation to HeapRegion 
from arguments.cpp).

I'll only need one extra review and then I'll push. Thanks,

Tony

Tony Printezis wrote:
> 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
>>>>>
>>>
>>
>

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