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

[Stefan Karlsson]

On 09/13/2013 11:21 AM, Thomas Schatzl wrote:
> 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.

I'll remove it.

>
> assert(MetaspaceSize <= MaxMetaspaceSize, ...)

I'll add it.

>
>> 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.

Good point. Are you OK with me using my current approach then?

>
> 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.

I'm going to remove the FLAG_IS_DEFUALT check.

>
> 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.

Yeah. I don't want to spend ages writing this test. If we get a problem 
with this, we'll fix it then.

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

I can add one where I set the flags to 0.

thanks,
StefanK
>
> Thomas
>