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

[Thomas Schatzl]

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