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

[Yasumasa Suenaga]

Hi Thomas,

I uploaded new webrev:
  http://cr.openjdk.java.net/~ysuenaga/JDK-8217432/webrev.03/

I agree with you that inc_capacity_until_GC() returns additional
bool value whether new HWM is exceeded MaxMetaspaceSize.

I added `can_retry` to argument of inc_capacity_until_GC().
It will be set to false if `new_value` exceeds MaxMetaspaceSize.
Then inc_capacity_until_GC() returns false, but it is not break
assert() in compute_new_size() because inc_capacity_until_GC()
is called with `expand_bytes` which is limited by MaxMetaspaceSize.
(It is ensured by this change)

This change has passed vmTestbase/metaspace, gc/metaspace, and
submit repo tests.


Thanks,

Yasumasa

2019年1月30日(水) 23:55 Thomas Schatzl <thomas.schatzl at oracle.com>:
>
> Hi,
>
> On Wed, 2019-01-30 at 22:16 +0900, Yasumasa Suenaga wrote:
> > Thanks Thomas!
> >
> > I uploaded new webrev:
> >    http://cr.openjdk.java.net/~ysuenaga/JDK-8217432/webrev.02/
> >
> > But I guess we can remove the following change:
> >
> > --------------
> > @@ -1477,6 +1483,9 @@
> >     // the HWM, an allocation is still attempted. This is because
> > another thread must then
> >     // have incremented the HWM and therefore the allocation might
> > still succeed.
> >     do {
> > +    if (MetaspaceGC::capacity_until_GC() + delta_bytes >
> > MaxMetaspaceSize) {
> > +      return NULL;
> > +    }
> >       incremented = MetaspaceGC::inc_capacity_until_GC(delta_bytes,
> > &after, &before);
> >       res = allocate(word_size, mdtype);
> >     } while (!incremented && res == NULL);
> > --------------
> >
> > OTOH inc_capacity_until_GC() shouldn't be passed the value which
> > exceeds MaxMetaspaceSize.
> > Should we keep this change?
>
> Actually this code (either my suggestion or yours) does not work either
> - if we read capacity_until_GC() here and do the check, in the re-read
> in inc_capacity_until_gc() to calculate the new value may be different
> (may have been updated in the meantime).
>
> I was not looking at the loop in detail which is broken as well,
> assuming like other code we just fixed that allocation is always
> possible.
>
> One option would be to have inc_capacity_until_gc() return an
> additional bool value whether MaxMetaspaceSize has already been
> exceeded (i.e. there is a chance that any further attempt is
> successful), and exit this this loop if not so.
>
> This is different to the initial approach where the method returned
> false as result because the code can fail to set a new value due to
> races anyway, but still be able to retry.
>
> Another option would be to have inc_capacity_until_gc() return true to
> fake a successful increment if MaxMetaSpaceSize had already been
> reached. This is a bit sneaky though so I would prefer the former
> option. What do you think?
>
> Thanks,
>   Thomas
>
>