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

[Tony Printezis]

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

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.

I also looked at the webrev and it looks fine. 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?

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

-- 
Tony Printezis | Staff Software Engineer | Twitter

@TonyPrintezis
tprintezis at twitter.com

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130829/2687d6b0/attachment.htm>