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

[Tony Printezis]

For some reason, this message never arrived in my inbox. Trying again...

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