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

[Jesper Wilhelmsson]

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 <mailto: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/
>>>     <http://cr.openjdk.java.net/%7Ejwilhelm/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/
>>>             <http://cr.openjdk.java.net/%7Ejwilhelm/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/74f7c609/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jesper_wilhelmsson.vcf
Type: text/x-vcard
Size: 247 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130116/74f7c609/jesper_wilhelmsson.vcf>