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.org/pipermail/hotspot-gc-dev/attachments/20130116/17638dbd/attachment.htm>
More information about the hotspot-gc-dev
mailing list