RFR (S): 8006088: Incompatible heap size flags accepted by VM

[John Cuthbertson]

Hi Thomas,

The change looks good to me. Good comment about the alignment of 
OldSize. The tests look great.

JohnC

On 3/22/2013 5:12 AM, Thomas Schatzl wrote:
> Hi all,
>
>    ping?
>
> On Mon, 2013-03-18 at 13:53 +0100, Thomas Schatzl wrote:
>> Hi all,
>>
>> On Thu, 2013-03-14 at 11:17 +0100, Mikael Gerdin wrote:
>>> Thomas,
>>>
>>> On 2013-03-14 09:28, Thomas Schatzl wrote:
>>>> Hi all,
>>>>
>>>>     please have a look at the following change which tries to make heap
>>>> size argument processing more intuitive.
>>>>
>>>> The second two are no bugs imo as the user did not mention a bound for
>>>> the maximum heap size in the arguments, so it would be free to choose
>>>> anything >= InitialHeapSize.
>>>>
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~tschatzl/8006088/webrev/
>>>>
>>>> The problem with the VM starting up when InitialHeapSize > MaxHeapSize
>>>> is fixed in the first hunk of collectorPolicy.cpp where the respective
>>>> sanity check has been added.
>>>>
>>>> The change for the "incompatible minimum and initial heap size is located
>>>> in arguments.cpp. I simply made the processing of -XX:InitialHeapSize the
>>>> same as -Xms and -XX:MaxHeapSize the same as -Xmx.
>>>>
>>>> The latter for consistency (using the same code path for -Xmx and
>>>> -XX:MaxHeapSize), as at the moment it does not set any internal variables.
>>>>
>>>> The issues in (3) are fixed in the last two hunks of collectorPolicy.cpp
>>>> where if New- and OldSize are greater than MaxHeapSize *and* MaxHeapSize
>>>> has been set on the command line, New- and OldSize are shrunk to fit.
>>> The code looks like it does what you describe :)
>>>
>>> Since the arguments parsing code is kind of hard to follow IMO and
>>> seemingly hard to get right it might be a good idea to write a
>>> regression test for this.
>>>
>>> Just writing a short jtreg test to launch vms with these arg
>>> combinations that you described above and verify that we get the
>>> expected outcome should not be too much code with the ProcessTools test
>>   done, added jtreg tests.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~tschatzl/8006088/webrev.1/
>>
>> Note that the code additionally removes two asserts that are imo invalid
>> to do at this point.
>>
>> I.e.
>>
>> assert(max_heap >= InitialHeapSize, "Error");// or >= OldSize in 7196080
>> assert(max_heap >= NewSize, "Error");
>>
>> The max_heap >= OldSize assert is problematic because its default value
>> is 4M. So again, if you set MaxHeapSize to e.g. 1-3M, the assert will
>> fail.
>>
>> The same applies to NewSize with a sufficiently small heap.
>>
>> There is the new sanity check collectorPolicy.cpp that checks whether
>> MaxHeapSize >= InitialHeapSize also in product mode (i.e. conflicting
>> command line params have been set).
>>
>> Additionally, NewSize and OldSize are adjusted to MaxHeapSize (or
>> MaxHeapSize to NewSize + OldSize) later in the collector policy too.
>>
>> These asserts also cause different behavior with/without debug mode.
> Note that in 7196080 these asserts have already been removed.
>
> Thomas
>
>
>