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

Jesper Wilhelmsson jesper.wilhelmsson at oracle.com
Wed Jan 16 15:36:38 UTC 2013


On 2013-01-16 15:05, Bengt Rutisson wrote:
> 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.

Checking for != 0 is enough to protect the division but I can change it to > 0 
if you have strong feelings about it. The flag verification will catch that -1 
is an invalid number for NewRatio. The fact that it's not caught in G1 is a 
bug in G1 that already exist today and is unrelated to this change.

I agree that NewRatio should be unsigned, but that's a separate bug. I created 
JDK-8006432 for that.

https://jbs.oracle.com/bugs/browse/JDK-8006432

> 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.

Actually no. The two negative NewRatio will be multiplied and give a positive 
answer: (OldSize / NewRatio) * (NewRatio + 1)
The corner case -1 will return 0.
But it really doesn't matter what the function returns if NewRatio is zero or 
negative since the VM will abort shortly after anyway. (I bluntly ignore the 
G1 bug in this statement.)

> 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 ;)

The entire collector policy mess needs a round with a sledge hammer as I 
mentioned in my reply to Jon earlier in this mail thread. That is however a 
larger change that I think is unrelated to this change. OK, not completely 
unrelated, but it deserves its own CR.

The crash below is unrelated to this change and will happen with your 
suggested NewRatio > 0 as well.
/Jesper

>
>  > 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 --------------
A non-text attachment was scrubbed...
Name: jesper_wilhelmsson.vcf
Type: text/x-vcard
Size: 247 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20130116/9d0a5c54/jesper_wilhelmsson.vcf>


More information about the hotspot-gc-dev mailing list