Code Review Request: 6819085 G1: use larger and/or user settable region size
john cuthbertson - Sun Microsystems
John.Cuthbertson at Sun.COM
Wed Aug 5 17:15:24 PDT 2009
The changes look good to me. I do have one question: should we change
"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?
On 08/05/09 09:28, Tony Printezis wrote:
> Thanks for the review! See inline.
> Jon Masamitsu wrote:
>> 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).
>> 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.
>> 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
> // 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:
> Apart from the changes Jon suggested, I also added a couple of casts
> to keep gcc happy.
>> Otherwise, looks good.
>> On 07/30/09 14:21, Tony Printezis wrote:
>>> The idea is to allow the user to set the region size with a
>>> parameter (-XX:G1HeapRegionSize=N).
More information about the hotspot-gc-dev