RFR (S): JDK-6348447 - Specifying -XX:OldSize crashes 64-bit VMs
Erik Helin
erik.helin at oracle.com
Wed Jan 23 16:19:56 UTC 2013
Jesper,
thanks for updating the code, see my comments inline.
On 01/23/2013 02:43 PM, Jesper Wilhelmsson wrote:
> Erik,
>
> Thanks for looking at it again. An updated webrev can be found here:
> http://cr.openjdk.java.net/~jwilhelm/6348447/webrev.4/
>
> See comments inline.
>
> On 21/1/13 11:12 AM, Erik Helin wrote:
>> A couple of comments:
>>
>> - Instead of having the comment:
>> > "NewRatio is checked earlier and can not be zero here"
>> Could you add an assert that says:
>> > assert(NewRatio != 0, "Should have been checked earlier");
>> This way you get a verifiable comment :)
> Agreed and fixed.
Thanks, looks good!
On 01/23/2013 02:43 PM, Jesper Wilhelmsson wrote:
> On 21/1/13 11:12 AM, Erik Helin wrote:
>> - I think its better if the calculation
>> > (OldSize / NewRatio) * (NewRatio + 1)
>> is saved in a variable, perhaps:
>> > uintx heap_size_by_scaling_old_size =
>> > (OldSize / NewRatio) * (NewRatio + 1);
>> >
>> > MaxHeapSize = heap_size_by_scaling_old_size;
>> > InitialHeapSize = heap_size_by_scaling_old_size;
> Agreed and fixed.
Looks good as well!
On 01/23/2013 02:43 PM, Jesper Wilhelmsson wrote:
> On 21/1/13 11:12 AM, Erik Helin wrote:
>> - This is not related to your change, but I would prefer if some of
>> the logic in TwoGenerationCollectorPolicy::adjust_gen0_size got some
>> more descriptive names, perhaps:
>> > bool is_heap_too_small =
>> > (*gen1_size_ptr + *gen0_size_ptr) > heap_size;
>> >
>> > if (is_heap_too_small) {
>> > bool has_heap_space_left_for_gen0 =
>> > heap_size >= (min_gen1_size + min_alignment());
>> > bool is_gen0_too_large =
>> > heap_size < (*gen0_size_ptr + min_gen1_size);
>> >
>> > if (is_gen0_too_large && has_heap_space_left_for_gen0) {
>> > ....
>> > }
> I guess this is a matter of taste. I would actually prefer to keep it as
> is. I do appreciate naming parts of complex expressions into local
> variables to increase readability, but I don't find this expression
> complex enough to motivate it.
Sure, this is a matter of taste, each to his own. I prefer the
variables, but if a second reviewer also prefers to not introduce the
variables, then I'm fine with that as well.
On 01/23/2013 02:43 PM, Jesper Wilhelmsson wrote:
> On 21/1/13 11:12 AM, Erik Helin wrote:
>> - As a final possible cleanup of
>> TwoGenerationCollectorPolicy::adjust_gen0_size, I think the variable:
>> > bool result = false;
>> can be removed and instead an early exit can be used in the if
>> statement that sets "result" to true and the method can return false
>> in the end.
> Again a matter of taste. At some point I was told that it was
> discouraged to use early exit. I don't remember where I heard/read it,
> but until we have some guidelines to dictate early exit I prefer using a
> result variable. Random return statements are easily overlooked when
> browsing code.
I agree in the common case that early exits can cause problems, but when
a function is as small as this one, then I think it increases
readability. Again, let a second reviewer have an opinion about this. If
the decision is to keep the variable, then I'm fine with that :)
Thanks,
Erik
> /Jesper
>
>>
>> What do you think of these suggestions?
>>
>> Thanks,
>> Erik
>>
>>> /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
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
More information about the hotspot-gc-dev
mailing list