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

[Thomas Stüfe]

Hi Yasumasa,

looks good to me as well.

I like the added comment but would suggest making it more precise: "Try to
increase metaspace size by v bytes" reads like it does anything of
substance like allocating memory etc, but it does nothing but increase the
limit variable. How about this:

"Try to increase the _capacity_until_GC limit counter by v bytes.
 Returns true if it succeeded. It may fail if either another thread
concurrently increased the limit or the new limit would be larger than
MaxMetaspaceSize."

Maybe also indicate that can_retry is only defined if function returns
false, new_cap_until_GC and old_cap_until_GC only if return value is true:

- // Optionally returns new and old metaspace capacity in
+ // On success, optionally returns new and old metaspace capacity in

- // Optionally sets can_retry to indicate whether if there is actually
+ // On error, optionally sets can_retry to indicate whether if there is
actually

If you change the comment, I am fine with the patch as it is and do not
need another webrev.

Thank you, Thomas






On Thu, Jan 31, 2019 at 3:00 PM Yasumasa Suenaga <yasuenag at gmail.com> wrote:

> Thanks Thomas!
>
> I will add the comment to inc_capacity_until_GC() declaration.
>
> I'm waiting for second reviewer.
>
>
> Yasumasa
>
>
> On 2019/01/31 18:16, Thomas Schatzl wrote:
> > Hi Yasumasa,
> >
> > On Thu, 2019-01-31 at 14:09 +0900, Yasumasa Suenaga wrote:
> >> 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.
> >
> >    looks good to me. Can you add some documentation to the declaration
> > of inc_capacity_until_GC() like:
> >
> > // Try to increase metaspace size by v bytes. Returns true if
> > // succeeded, false if not due to competing threads trying.
> > // Optionally returns new and old metaspace capacity in
> > // new_cap_until_GC and old_cap_until_GC respectively.
> > // Optionally sets can_retry to indicate whether if there is actually
> > // enough space remaining to satisfy the request.
> >
> > No need for a re-review for that (or something similar potentially
> > better worded description.
> >
> > Thanks,
> >    Thomas
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20190201/5765ba69/attachment.htm>