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

[Yasumasa Suenaga]

Thank you for your comment.
I uploaded new webrev (it passed all tests on submit repo):

   http://cr.openjdk.java.net/~ysuenaga/JDK-8217432/webrev.01/

I write reply based on it as below.

On 2019/01/28 22:55, Thomas Schatzl wrote:
> 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.

I added assert() to inc_capacity_until_GC() because _capacity_until_GC
shouldn't exceed MaxMetaspaceSize.
This function will not return false when _capacity_until_GC attempt to
be exceeded MaxMetaspaceSize.


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

Your suggestion seems to be equivalent to my change.

-------------
    const double min_tmp = used_after_gc / maximum_used_percentage;
    size_t minimum_desired_capacity =
-    (size_t)MIN2(min_tmp, double(max_uintx));
+    (size_t)MIN2(min_tmp, double(MaxMetaspaceSize));
    // Don't shrink less than the initial generation size
    minimum_desired_capacity = MAX2(minimum_desired_capacity,
                                    MetaspaceSize);
-------------

I don't understand why current implementation use `max_uintx` at this.
I guess it means "max value of Metaspace", so I think it should use
MaxMetaspaceSize rather than max_uintx.


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

It is hard to reproduce this problem because this issue is caused by
C++ member variable (MetaspaceGC::_capacity_until_GC).

It can be tested with WhiteBox test (incMetaspaceCapacityUntilGC()),
however it tests inc_capacity_until_GC() call only.

OTOH we might test as Aleksey said, but we can't estimate precise
commit memory size of Metaspace, and evaluate why OOME is occurred.
(memory consumption or invalid _capacity_until_GC)

As I said before, I added assert() to inc_capacity_until_GC().
It will fail without other change in this webrev. So I think we can evaluate
it via vmTestbase/metaspace/shrink_grow/ShrinkGrowTest/ShrinkGrowTest.java .


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

I added the code to check total amount of _capacity_until_GC
before inc_capacity_until_GC() call.
But I concern the check and increment are not atomically. Is it okay?


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

Comments are welcome!


Thanks,

Yasumasa


> Thanks,
>    Thomas
> 
>