RFR (S): JDK-6348447 - Specifying -XX:OldSize crashes 64-bit VMs
Erik Helin
erik.helin at oracle.com
Mon Jan 21 10:12:29 UTC 2013
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!
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 :)
- 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;
- 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) {
> ....
> }
- 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.
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