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

[Thomas Schatzl]

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.

Thomas