RFR (S): 8006088: Incompatible heap size flags accepted by VM
john.cuthbertson at oracle.com
Fri Mar 22 10:24:21 PDT 2013
The change looks good to me. Good comment about the alignment of
OldSize. The tests look great.
On 3/22/2013 5:12 AM, Thomas Schatzl wrote:
> Hi all,
> 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:
>>> 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.
>>>> 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.
>> Note that the code additionally removes two asserts that are imo invalid
>> to do at this point.
>> 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
>> 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.
More information about the hotspot-gc-dev