PING: RFR: 8217432: MetaspaceGC::_capacity_until_GC exceeds MaxMetaspaceSize

[Thomas Schatzl]

Hi Yasumasa,

On Fri, 2019-01-25 at 11:35 +0900, Yasumasa Suenaga wrote:
> PING: Could you review this?
> 
> 
> Yasumasa
> 
> 
> On 2019/01/21 17:52, Yasumasa Suenaga wrote:
> > Hi all,
> > 
> > I filed this issue to JBS:
> >    https://bugs.openjdk.java.net/browse/JDK-8217432
> > 
> > I investigated it, and I found that `minimum_desired_capacity` and
> > `maximum_desired_capacity` in `MetaspaceGC::compute_new_size()`
> > might exceed MaxMetaspaceSize.
> > They shouldn't exceed MaxMetaspaceSize.
> > 
> > I uploaded webrev for this bug. Could you review it?
> >    http://cr.openjdk.java.net/~ysuenaga/JDK-8217432/webrev.00/
> > 
> > This change has passed all tests on submit repo,
> > :vmTestbase_vm_metaspace,
> > and :vmTestbase_nsk_sysdict jtreg tests.

The suggested change does not work - you can't return false from
MetaspaceGC::inc_capacity_until_gc() because the return value indicates
whether it was possible to change the value, not whether the value
passed sense.

The code before that must make sure that the resulting value is within
limits (that could be a good assert in inc_capacity_until_GC()) from my
understanding, without being an expert here.

So with the current change, the assert in metaspace.cpp:260 would
simply fire in this case.

Making sure that the HWM does not go over MaxMetaspaceSize could be
achieved by limiting expand_bytes passed to the method; I *think*,
without following where minimum_desired_capacity is also used in that
method, clamping it via

241:   minimum_desired_capacity = MIN2(MAX2(minimum_desired_capacity,
MetaspaceSize), MaxMetaspaceSize);

would achieve the desired effect.

Could you also provide a test case for this change (i.e. failing
before, good after) as Aleksey suggested?

Note that my suggestion only fixes the call to inc_capacity_until_gc()
in MetaspaceGC::compute_new_size(). I very much believe it is required
to add another guard in ClassLoaderMetaspace::expand_and_allocate().

(Please add the assert in inc_capacity_until_gc() I mentioned above!).

Maybe Thomas Stüfe can comment on this a bit because he looks to be
having done a lot in that area. ;)

Thanks,
  Thomas