RFR: 8217432: MetaspaceGC::_capacity_until_GC exceeds MaxMetaspaceSize

[Yasumasa Suenaga]

Hi Aleksey,

On 2019/01/21 18:54, Aleksey Shipilev wrote:
> On 1/21/19 9:52 AM, Yasumasa Suenaga wrote:
>> 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.
> 
> Yes, looks like it. I was wondering in my recent Epsilon tests why
> setting very low MaxMetaspaceSize
> did not OOM. With your patch, it OOMs on low MMS, as expected.
> 
> I think you can construct the non-GC-specific regression test. Take
> the sample class generator from
> here:
> http://hg.openjdk.java.net/jdk/jdk/file/tip/test/hotspot/jtreg/gc/epsilon/TestClasses.java
> and
> feed different MaxMetaspaceSize settings via ProcessTools, like here:
> http://hg.openjdk.java.net/jdk/jdk/file/tip/test/hotspot/jtreg/runtime/Metaspace/MaxMetaspaceSizeTest.java

I think we can test it with test/hotspot/jtreg/vmTestbase/metaspace/shrink_grow/ShrinkGrowTest/ShrinkGrowTest.java .
I've confirmed ShrinkGrowTest works fine on my Linux x64 box.


>> I uploaded webrev for this bug. Could you review it?
>> ? http://cr.openjdk.java.net/~ysuenaga/JDK-8217432/webrev.00/
> 
> It is weird to see this block in expand_and_allocate:
> 
> 1498     if (!incremented && SafepointSynchronize::is_at_safepoint()) {
> 1499       // Cannot expand metaspace more.
> 1500       return NULL;
> 1501     }
> 
> Maybe we should instead fall-through in inc_capacity_until_GC?

No.
If MetaspaceGC::inc_capacity_until_GC() returns NULL, new metadata
allocation might be failed because Metaspace does not have enough space.

   while (!incremented && res == NULL)

Thus above condition will be `true` forever :-)
(inc_capacity_until_GC() is false && cannot get memory from Metaspace)


You can see this behavior with ShrinkGrowTest.
But you need to attach GDB to it.


Thanks,

Yasumasa



> 141   if (new_value < old_capacity_until_GC) {
> 142     // The addition wrapped around, set new_value to aligned max value.
> 143     new_value = align_down(max_uintx, Metaspace::commit_alignment());
> 144   }
> 145
> 146   if (new_value > MaxMetaspaceSize) {
> 147     return false; // instead, new_value = MIN2(new_value, MaxMetaspaceSize);
> 148   }
> 149
> 
> ...that would naturally exit the loop, AFAIU.
> 
> Thanks,
> -Aleksey
>