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

[Jesper Wilhelmsson]

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/

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/
>>
>> 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.org/pipermail/hotspot-gc-dev/attachments/20130115/268ae372/jesper_wilhelmsson.vcf>