Review request: 8024650: Don't adjust MaxMetaspaceSize up to MetaspaceSize

[Thomas Schatzl]

Hi,

On Fri, 2013-09-13 at 10:58 +0200, Bengt Rutisson wrote:
> Hi Stefan,
> 
> On 9/12/13 5:18 PM, Stefan Karlsson wrote:
> > http://cr.openjdk.java.net/~stefank/8024650/webrev.00/
> >
> > - Limit MetaspaceSize to MaxMetaspaceSize
> > - Make sure we don't align down to 0.
> > - jtreg test
> >
> > Note that this patch also adds/changes some functionality in the 
> > OutputAnalyzer in test/testlibrary.
> >
> > thanks,
> > StefanK
> 
> One minor thing is that I think we could remove this line in 
> collectorPolicy.cpp:
> 
>    72   if (!FLAG_IS_DEFAULT(MaxMetaspaceSize)) {
> 
> Doing that will make sure that we always have MetaspaceSize <= 
> MaxMetaspaceSize, which I think is the case already in the code, but 
> this would make it clearer.

Actually I think it is required to be removed.

What if you set only MetaspaceSize to some value above MaxMetaspaceSize?

Maybe a some assert at the end of the method that verifies this
requirement should be added, e.g.

assert(MetaspaceSize <= MaxMetaspaceSize, ...)

> About the test. Instead of parsing the PrintFlagsFinal output you could 
> use ManagementFactoryHelper.getDiagnosticMXBean().getVMOption(" 
> MaxMetaspaceSize") etc. 

If I am not mistaken, the test starts another VM and wants to check the
values of the other VM.

The method you proposed gets the values of the current VM only.

The test is also missing subtests when not setting
MetaspaceSize/MaxMetaspaceSize at all. I.e. it does not stress any code
using FLAG_IS_DEFAULT, if I read it correctly.

To determine the "conservative" max heap alignment the change could add
a whitebox method providing it - the code has just been checked in -
instead of defining it as a constant. You add -XX:-UseLargePages to the
command line though.

There are also no (eventually failing) tests for boundaries, i.e.
setting any of the variables to zero or max_uintx.

Thomas