PING: RFR: 8217432: MetaspaceGC::_capacity_until_GC exceeds MaxMetaspaceSize
yasuenag at gmail.com
Tue Jan 29 12:39:53 UTC 2019
Thank you for your comment.
I uploaded new webrev (it passed all tests on submit repo):
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?
>> On 2019/01/21 17:52, Yasumasa Suenaga wrote:
>>> Hi all,
>>> I filed this issue to JBS:
>>> 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?
>>> This change has passed all tests on submit repo,
>>> 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,
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!
More information about the hotspot-gc-dev