RFR: 8226236: [TESTBUG] win32: gc/metaspace/TestCapacityUntilGCWrapAround.java fails

[Yasumasa Suenaga]

On Wed, 14 Oct 2020 08:59:51 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> I found @shipilev's analysis in the bug report and understand better what happens now.
>> 
>> I do not think this is a test error, the test is correct and shows a real issue. Just pasting my JBS comment here:
>> 
>> The test verifies that calling MetaspaceGC::inc_capacity_until_GC() repeatedly will not overflow the gc threshold.
>> 
>> 1) MetaspaceGC::ergo_initialize()
>> 
>> MaxMetaspaceSize, if left unspecified, is supposed to be "infinite". In reality it defaults to max_uintx. But it gets
>> aligned down by Metaspace::reserve_alignment. That one is either one of os::vm_allocation_granularity or 4 pages,
>> whatever is larger. The 4 pages thing I recently added with JDK-8245707 to shake loose misuse of this alignment value
>> (exactly cases like this).  So on Windows, Metaspace::reserve_alignment has always been os::vm_allocation_granularity
>> (64K). On Linux, it was always 4K, until recently it switched to 16K.  This explains the 16K aligned value we see for
>> MaxMetaspaceSize in Alexeys test: 4294950912 (FFFFC000).
>> 2) MetaspaceGC::inc_capacity_until_GC()
>> 
>> The increase value causes an overflow - its supposed to do that. Overflow gets handled by:
>> 
>>   if (new_value < old_capacity_until_GC) {
>>     // The addition wrapped around, set new_value to aligned max value.
>>     new_value = align_down(max_uintx, Metaspace::commit_alignment());
>>   }
>> 
>> which sets the new threshold at max_uintx aligned down by Metaspace::commit_alignment(), which is just os::vm_page size.
>> 
>> So the new value is 4294963200, resp. FFFFF000
>> 
>> So the problem is that one value gets aligned down by Metaspace::reserve_alignment() (os::vm_allocation_granularity()),
>> the other by Metaspace::commit_alignment (os::vm_page_size()). Then we compare those values.
>> So the test is okay. It showed us a real issue. I am not 100% sure what the correct behavior would be. Maybe increasing
>> the gc threshold beyond MaxMetaspaceSize should not be an error at all, but we should just cap out at that value?
>> Possible solutions:
>> 
>> A) As stated above, just cap.
>> 
>> b) All that alignment business is actually unnecessary and we could remove it for clarity after JEP387 is out of the
>> door. MaxMetaspaceSize does not need to be aligned to anything, neither does the GC threshold.
>> C) as a small workaround, at (2) one probably could use Metaspace::reserve_alignment, not commit_alignment. That makes
>> no sense semantically but would remove this error.
>> (This must have been always a problem on Windows, and only recently on Linux, right?)
>
> A minimal fix - which preserves the current behavior as much as possible - could be:
> 
> --- a/src/hotspot/share/memory/metaspace.cpp
> +++ b/src/hotspot/share/memory/metaspace.cpp
> @@ -152,7 +152,7 @@ bool MetaspaceGC::inc_capacity_until_GC(size_t v, size_t* new_cap_until_GC, size
>  
>    if (new_value < old_capacity_until_GC) {
>      // The addition wrapped around, set new_value to aligned max value.
> -    new_value = align_down(max_uintx, Metaspace::commit_alignment());
> +    new_value = align_down(max_uintx, Metaspace::reserve_alignment());
>    }
>  
>    if (new_value > MaxMetaspaceSize) {
> 
> This preserves the current behavior:
> - increasing the gc threshold beyond MaxMetaspaceSize should be an error
> - on 32bit, increasing the gc threshold beyond the scope of the 32bit counter should be tolerated and result in a value
>   capped at the end of 32bit; unless MaxMetaspaceSize is set and lower than max, in which case it would be an error.
> 
> Note that I have no possibility to test this since the 32bit build on Windows still seems broken.

Thanks @tstuefe for the fix! I will merge it to this PR.

> (This must have been always a problem on Windows, and only recently on Linux, right?)

I don't know how long this problem has been available, but I saw this on Linux x64 when I removed `vm.bits == 32`. It
is caused by new_value exceeds MaxMetaspaceSize.

I think we should be current behavior should be preserved. I added the logic which relates to `_capacity_until_gc`  in
[JDK-8217432](https://bugs.openjdk.java.net/browse/JDK-8217432). Before this fix, I saw OutOfMemoryError despite that
the memory was still available. And also @shipilev seems to had seen [the problems with Metaspace on
Epsilon](https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2019-January/024595.html).

BTW TestCapacityUntilGCWrapAround.java still should be run on 32bit platforms only? I think we should check whether
`_capacity_until_gc`  exceeds MaxMetaspaceSize on 64bit platforms.

-------------

PR: https://git.openjdk.java.net/jdk/pull/628