RFR (S): JDK-6348447 - Specifying -XX:OldSize crashes 64-bit VMs

[Vitaly Davidovich]

Looks good Jesper.  Maybe just a comment there that NewRatio hasn't been
checked yet but if it's 0, VM will exit later on anyway - basically, what
you said in the email :).

Cheers

Sent from my phone
On Jan 16, 2013 7:49 AM, "Jesper Wilhelmsson" <jesper.wilhelmsson at oracle.com>
wrote:

>
> On 2013-01-16 09:23, Bengt Rutisson wrote:
>
> On 1/15/13 2:41 PM, Jesper Wilhelmsson wrote:
>
> On 2013-01-15 14:32, Vitaly Davidovich wrote:
>
> Hi Jesper,
>
> Is NewRatio guaranteed to be non-zero when used inside
> recommended_heap_size?
>
> As far as I can see, yes. It defaults to two and is never set to zero.
>
>
> No, there is no such guarantee this early in the argument parsing. The
> check to verify that NewRatio > 0 is done in
> GenCollectorPolicy::initialize_flags(), which is called later in the start
> up sequence than your call to CollectorPolicy::recommended_heap_size() and
> it is never called for G1.
>
> Running with your patch crashes:
>
> java -XX:OldSize=128m -XX:NewRatio=0 -version
> Floating point exception: 8
>
>
> Oh, yes, you're right. Sorry!
>
> Good catch Vitaly!
>
> New webrev:
> http://cr.openjdk.java.net/~jwilhelm/6348447/webrev.3
>
> I'm just skipping the calculation if NewRatio is zero. The VM will abort
> anyway as soon as it realizes that this is the case.
> /Jesper
>
>
>  Bengt
>
> /Jesper
>
>  Thanks
>
> Sent from my phone
> On Jan 15, 2013 8:11 AM, "Jesper Wilhelmsson" <
> jesper.wilhelmsson at oracle.com> wrote:
>
>> Jon,
>>
>> Thank you for looking at this! I share your concerns and I have moved the
>> knowledge about policies to CollectorPolicy. set_heap_size() now simply
>> asks the collector policy if it has any recommendations regarding the heap
>> size.
>>
>> Ideally, since the code knows about young and old generations, I guess
>> the new function "recommended_heap_size()" should be placed in
>> GenCollectorPolicy, but then the code would have to be duplicated for G1 as
>> well. However, CollectorPolicy already know about OldSize and NewSize so I
>> think it is OK to put it there.
>>
>> Eventually I think that we should reduce the abstraction level in the
>> generation policies and merge CollectorPolicy, GenCollectorPolicy and maybe
>> even TwoGenerationCollectorPolicy and if possible G1CollectorPolicy, so I
>> don't worry too much about having knowledge about the two generations in
>> CollectorPolicy.
>>
>>
>> A new webrev is available here:
>> http://cr.openjdk.java.net/~jwilhelm/6348447/webrev.2/
>>
>> Thanks,
>> /Jesper
>>
>>
>>
>> On 2013-01-14 19:00, Jon Masamitsu wrote:
>>
>>> Jesper,
>>>
>>> I'm a bit concerned that set_heap_size() now knows about how
>>> the CollectorPolicy uses OldSize and NewSize.   In the distant
>>> past set_heap_size() did not know what kind of collector was
>>> going to be used and probably avoided looking at those
>>> parameters for that reason.  Today we know that a generational
>>> collector is to follow but maybe you could hide that knowledge
>>> in CollectorPolicy somewhere and have set_heap_size() call into
>>> CollectorPolicy to use that information?
>>>
>>> Jon
>>>
>>>
>>> On 01/14/13 09:10, Jesper Wilhelmsson wrote:
>>>
>>>> Hi,
>>>>
>>>> I would like a couple of reviews of a small fix for JDK-6348447 -
>>>> Specifying -XX:OldSize crashes 64-bit VMs
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~jwilhelm/6348447/webrev/
>>>>
>>>> Summary:
>>>> When starting HotSpot with an OldSize larger than the default heap size
>>>> one will run into a couple of problems. Basically what happens is that the
>>>> OldSize is ignored because it is incompatible with the heap size. A debug
>>>> build will assert since a calculation on the way results in a negative
>>>> number, but since it is a size_t an if(x<0) won't trigger and the assert
>>>> catches it later on as incompatible flags.
>>>>
>>>> Changes:
>>>> I have made two changes to fix this.
>>>>
>>>> The first is to change the calculation in
>>>> TwoGenerationCollectorPolicy::adjust_gen0_sizes so that it won't result in
>>>> a negative number in the if statement. This way we will catch the case
>>>> where the OldSize is larger than the heap size and adjust the OldSize
>>>> instead of the young size. There are also some cosmetic changes here. For
>>>> instance the argument min_gen0_size is actually used for the old generation
>>>> size which was a bit confusing initially. I renamed it to min_gen1_size
>>>> (which it already was called in the header file).
>>>>
>>>> The second change is in Arguments::set_heap_size. My reasoning here is
>>>> that if the user sets the OldSize we should probably adjust the heap size
>>>> to accommodate that OldSize instead of complaining that the heap is too
>>>> small. We determine the heap size first and the generation sizes later on
>>>> while initializing the VM. To be able to fit the generations if the user
>>>> specifies sizes on the command line we need to look at the generation size
>>>> flags a little already when setting up the heap size.
>>>>
>>>> Thanks,
>>>> /Jesper
>>>>
>>>>
>>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130116/d556e663/attachment.htm>