RFR: 8025313: MetaspaceMemoryPool incorrectly reports undefined size for max

[Thomas Schatzl]

Hi,

On Mon, 2013-09-30 at 20:52 +0200, Jesper Wilhelmsson wrote:
> Thomas Schatzl skrev 30/9/13 5:50 PM:
> > Hi,
> >
> > On Mon, 2013-09-30 at 17:20 +0200, Erik Helin wrote:
> >> Hi all,
> >>
> >> this patch fixes an issue where the metaspace memory pool reports -1 for
> >> MemoryPoolMXBean.getUsage().getMax(), even though the user has set
> >> MaxMetaspaceSize on the command line.
> >>
> >> The problem is that we in collectorPolicy.cpp use FLAG_SET_ERGO when
> >> aligning MaxMetaspaceSize while at the same time relying on
> >> FLAG_IS_CMDLINE in memoryPool.cpp when deciding what to return for
> >> MetaspacePool::calculate_max_size.
> >>
> >> This patch removes FLAG_SET_ERGO and instead simply sets
> >> MaxMetaspaceSize.
> >
> > Why not check for max_uintx in MetaspacePool::calculate_max_size()? It
> > seems to me that the actual value is important here for returning it to
> > the MetaspaceMemoryPool, not where it came from.
> >
> > I mean, what if the user set a value of max_uintx on the commandline? It
> > always seems unintuitive to me when the user sets a flag to the default
> > value on the command line and the result is different than when not
> > setting anything.
> 
> I think there is a difference. Not setting a flag indicates that it is OK for 
> the VM to ergonomically change the value at will. Explicitly setting a flag 
> (even if it is to the default value) indicates that the user really wants this 
> value and ergonomics should try its best to leave it alone.
> /Jesper

That is true for argument processing, ergonomics and VM setup. But why
does it matter how the VM got to that value later when using the values
in its algorithms?

In this case, a value for MaxHeapSize of uintx_max indicates
"unlimited", but only if you do not specify it on the command line? If
you specify it, it means exactly uintx_max.

Also look at how this is later reported to the java code: if you do not
change anything (and -XX:+PrintFlagsFinal prints max_uintx), the java
application gets "-1".
If you set MaxHeapSize to the exactly same value you see printed there
manually, the application gets either 2^32-1 on 32 bit vms, or 2^63-1
(max_jlong)... (with some additional alignment thrown into the mix).

While I think it does not have much impact here except for the
programmer (both java application and VM) that needs to be aware of that
because uintx_max is a large value (but consider if "unlimited" were
defined as "0"), it seems surprising. And this distinction apparently
causes these kind of bugs :)

Having said that, I guess this has already been thought about anyway.

Back to the change: Looking at memoryPool.cpp, it seems the intended fix
may be 

  return !FLAG_IS_DEFAULT(MaxMetaspaceSize) ? MaxMetaspaceSize :
MemoryUsage::undefined_size(); 

instead of

  return FLAG_IS_CMDLINE(MaxMetaspaceSize) ? MaxMetaspaceSize :
MemoryUsage::undefined_size();

in MetaspacePool::calculate_max_size(). Other code in the same file also
uses this condition (ie. !FLAG_IS_DEFAULT() to distinguish between when
the user wants a limit or not; see line memoryPool.cpp:1290).
(This could be factored out as "user_wants_limit()" or so method maybe)

I think using !FLAG_IS_DEFAULT would also catch the case when ergonomics
changed MaxMetaspaceSize. The current solution throws away the
information who changed it. I.e. the ":" in the list of flags for
PrintFlagsFinal if I am not mistaken.

Thanks,
Thomas