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

[Jesper Wilhelmsson]

Hi,

Some further code inspection showed that it's possible to fix this bug in
TwoGenerationCollectorPolicy::initialize_flags() and keep the fix local. 
Thanks to Erik Helin who suggested this. A new webrev is available here:

http://cr.openjdk.java.net/~jwilhelm/6348447/webrev.3/

/Jesper


On 16/1/13 2:23 PM, Vitaly Davidovich wrote:
>
> 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 <mailto: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
>     <http://cr.openjdk.java.net/%7Ejwilhelm/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/20130119/a30d587a/attachment.htm>