RFR (S): 8019902: G1: Use the average heap size rather than the minimum heap size to calculate the region size

Thu Aug 29 15:19:44 UTC 2013

Hi again,

On 8/29/13 5:11 PM, Tony Printezis wrote:
> Hi there,
>
> If the avg scheme does something reasonable when only -Xmx is set (to 
> something large), I'm totally OK with it. I think I confused initial / 
> min size when I read your previous e-mail (still fetching that stuff 
> from cold long-term storage, it's taking some time).

Sure, no problem. The initial vs min size is confusing especially since 
there is no command line option to set the min size. If the initial is 
set we use that as a min size.

>
> For folks who set both -Xms and/or -Xmx and/or G1HeapRegionSize you 
> might want to consider generating a warning when the settings look 
> suspect, if you don't want to directly change them. But, if you want 
> to do that, a separate CR should be fine with it; it's kind of 
> orthogonal to this fix.

Right, I agree. We should warn if we change values but I don't think it 
should be done as part of this change.

>
> I also looked at the webrev and it looks fine.

Thanks!

> Out of curiosity:
>
> g1CollectorPolicy.cpp:
>
>  170   // Set up the region size and associated fields. Given that the
>  171   // policy is created before the heap, we have to set this up here,
>  172   // so it's done as soon as possible
>
> Why did you remove the period after possible?

Sorry. My bad. I added the period back. Thanks for catching that! :)

Bengt

>
> Tony
>
> On 8/29/13 10:21 AM, Bengt Rutisson wrote:
>> On 8/29/13 4:14 PM, Bengt Rutisson wrote:
>>>
>>> Tony,
>>>
>>> On 8/29/13 4:12 PM, Tony Printezis wrote:
>>>> Bengt,
>>>>
>>>> Doesn't what I said also applies for users who do not set -Xms? So, 
>>>> if -Xms is by default 6m, a user launches the VM with:
>>>>
>>>> java -Xmx32G ...
>>>>
>>>> and the region size is calculated to be 8m, what's the initial heap 
>>>> size (8m, i.e. one region)?
>>>
>>> No, by default we calculate a reasonable -Xms. So as long as the 
>>> policy uses that value and not the min heap size I think we are fine.
>>
>> Just to clarify a bit. In your example we get a -Xms of 128m:
>>
>> java -XX:+UseG1GC -XX:+PrintFlagsFinal -Xmx32G -version | grep 
>> InitialHeapSize
>>     uintx InitialHeapSize                          := 134217728       
>> {product}
>>
>>
>> With my fix this means that we will get a region size based on 128m + 
>> 32g / 2, which will turn out to be 8m. That gives us a minimum of 16 
>> regions (cool coincidence since that's what you suggested ;) ). And 
>> it will give us a maximum of about 4000 regions.
>>
>> Before my fix we would get a regions size of 1m, which means about 
>> 32000 regions.
>>
>> Bengt
>>
>>
>>
>>>
>>> Bengt
>>>
>>>>
>>>> Tony
>>>>
>>>> On 8/29/13 10:08 AM, Bengt Rutisson wrote:
>>>>>
>>>>> Hi Tony,
>>>>>
>>>>> Thanks for looking at this!
>>>>>
>>>>> Comments inline.
>>>>>
>>>>> On 8/29/13 3:03 PM, Tony Printezis wrote:
>>>>>> Hi Bengt,
>>>>>>
>>>>>> Yeah, I struggled with this heuristic when I did the original 
>>>>>> implementation of the heap region calculation. The issue only 
>>>>>> arises when the gap between the min and max heap size is very 
>>>>>> large. So, if someone launches the VM with:
>>>>>>
>>>>>> java -Xms32m -Xmx64g ...
>>>>>>
>>>>>> and G1 picks a region size of 8m, it'd start with only 4 regions 
>>>>>> which will probably make performance right at the beginning will 
>>>>>> be terrible (but I agree that it will be better as the heap 
>>>>>> grows, compared to if an 1m region size was used).
>>>>>
>>>>> Agreed. And just to be clear. The main problem with the existing 
>>>>> policy is that it by default always picks 1m regions if nothing is 
>>>>> set on the command line. This is due to the fact that it is not 
>>>>> based on the initial heap size (-Xms) but on the min heap size, 
>>>>> which by default is in the order of 6m. So, those who set -Xms on 
>>>>> the command line have experienced less of a problem. At least if 
>>>>> they set -Xms to high enough values.
>>>>>
>>>>>>
>>>>>> Can I suggest maybe an additional policy change? Use the avg to 
>>>>>> calculate the region size, as you proposed, but potentially 
>>>>>> adjust the min heap size based on a min region number (let's pick 
>>>>>> a number of a hat: 16; you might want to revise this). So, in the 
>>>>>> above example:
>>>>>>
>>>>>> -Xms32m -Xmx64g -> region size = 8m
>>>>>>
>>>>>> you'll actually adjust the min heap size 16 x 8m = 128m. This 
>>>>>> will avoid the potentially bad behavior right at the start. Of 
>>>>>> course, you'll start with a larger heap size than what the user 
>>>>>> asked for. On the other hand, if someone uses a huge max they 
>>>>>> probably expect the heap to grow. So starting with a large min 
>>>>>> might be OK.
>>>>>
>>>>> I see your point, but I don't really like the fact that if someone 
>>>>> explicitly sets -Xms on the command line we would ignore that and 
>>>>> use a value that is four times as large. Also, there is the 
>>>>> possibility to set the region size using G1HeapRegionSize on the 
>>>>> command line. So, in this use case I kind of think it would be 
>>>>> better to leave it up to the user to indicate if the heap is more 
>>>>> likely to be 32m or 64g by setting the region size explicitly.
>>>>>
>>>>> Thanks,
>>>>> Bengt
>>>>>
>>>>>>
>>>>>> Tony
>>>>>>
>>>>>> On 8/29/13 5:26 AM, Bengt Rutisson wrote:
>>>>>>>
>>>>>>> Hi everyone,
>>>>>>>
>>>>>>> Could I have a couple of reviews of this change:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~brutisso/8019902/webrev.00/
>>>>>>>
>>>>>>> The fact that G1 by default bases its region size on the minimum 
>>>>>>> heap size means that out of the box the region size will always 
>>>>>>> be 1M. This is a problem on large machines with lots of memory. 
>>>>>>> We pick a large heap size but get a very small region size. The 
>>>>>>> small regions are inefficient and cause a lot of memory 
>>>>>>> footprint. Normally we aim to get around 2048 regions, but on a 
>>>>>>> machine with a lot of memory we might pick a default max heap 
>>>>>>> size of 32G, which means that we will get ~32000 regions. This 
>>>>>>> can lead to out of memory situations - especially on Solaris x86.
>>>>>>>
>>>>>>> This patch changes the heuristics for picking the region size to 
>>>>>>> use the average between initial heap size (-Xms) and the maximum 
>>>>>>> heap size (-Xmx). This means that for large heaps we will pick 
>>>>>>> larger region sizes. In the 32G example we will now pick a 
>>>>>>> region size of 8m which means that we will have 4000 regions 
>>>>>>> which is more reasonable.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Bengt
>>>>>>
>>>>>
>>>>
>>>
>>
>