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

[Bengt Rutisson]

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

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/1521ae2c/attachment.htm>