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

Bengt Rutisson bengt.rutisson at oracle.com
Wed Jan 16 14:05:16 UTC 2013


On 1/16/13 1:45 PM, Jesper Wilhelmsson 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.

It is not enough to check NewRatio != 0 since it is a signed value. You 
should check NewRatio > 0. Or change the declaration of NewRatio from 
intx to uintx.

As it is now I think you will get a really huge result from 
recommended_heap_size() if someone sets -XX:NewRatio=-10 on the command 
line since you will be converting negative integers to a size_t value.

As I mentioned before the NewRatio > 0 check is not done for G1. But 
this should probably be considered a separate bug. On the other hand if 
we could make G1 inherit from GenCollectorPolicy it would do the 
NewRatio > 0 check and we would also have a better place to put the 
OldSize checks that you want to add. Two bugs for the price of one ;)

 > java  -XX:+UseG1GC -XX:NewRatio=-1 -version
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGFPE (0x8) at pc=0x000000010f5468a8, pid=31206, tid=4611
#
# JRE version:  (8.0-b68) (build )
# Java VM: Java HotSpot(TM) 64-Bit Server VM (25.0-b16-internal-jvmg 
mixed mode bsd-amd64 compressed oops)
# Problematic frame:
# V  [libjvm.dylib+0x6618a8] G1YoungGenSizer::heap_size_changed(unsigned 
int)+0x14a
#
# Failed to write core dump. Core dumps have been disabled. To enable 
core dumping, try "ulimit -c unlimited" before starting Java again
#
# An error report file with more information is saved as:
# /Users/brutisso/repos/hs-gc/hs_err_pid31206.log
#
# If you would like to submit a bug report, please visit:
#   http://bugreport.sun.com/bugreport/crash.jsp
#
Current thread is 4611
Dumping core ...
Abort trap: 6


Bengt

> /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.java.net/pipermail/hotspot-gc-dev/attachments/20130116/17638dbd/attachment.htm>


More information about the hotspot-gc-dev mailing list