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

[Thomas Stuefe]

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

>> I think TestCapacityUntilGCWrapAround.java has two problems:
>> 
>> 1. It might attempt to increase metaspace size over MaxMetaspaceSize (this PR fixes it)
>> 2. Overflow test would always fail because `MetaspaceGC::inc_capacity_until_GC()` always returns `false`
>> 
>> This test has introduced in JDK-8049599, but I cannot know details because JBS ticket is closed, but we can see its
>> commit: https://github.com/openjdk/jdk/commit/6f4355a3a6be2482c388c2022fc7c3c4b2f481c5   I enabled this test on 64bit
>> platforms because these problems might happen on them.
>> If you agree with the fix for 1. , I will add the test for 2. to this PR.
>
> 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.

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

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