RFR (S): JDK-6348447 - Specifying -XX:OldSize crashes 64-bit VMs
Jesper Wilhelmsson
jesper.wilhelmsson at oracle.com
Wed Jan 23 13:43:45 UTC 2013
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:
> Jesper,
>
> On 01/19/2013 06:38 PM, Jesper Wilhelmsson wrote:
>> Some further code inspection showed that it's possible to fix this
>> bug in
>> TwoGenerationCollectorPolicy::initialize_flags() and keep the fix local.
>
> I think this is much better, nice work!
Thanks for the suggestions!
>
> On 01/19/2013 06:38 PM, Jesper Wilhelmsson wrote:
>> A new webrev is available here:
>>
>> http://cr.openjdk.java.net/~jwilhelm/6348447/webrev.3/
>
> 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.
> - 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.
> - 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.
> - 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.
/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