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

[Yasumasa Suenaga]

On Tue, 13 Oct 2020 20:02:56 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Originally filed at AdoptOpenJDK:
>> https://github.com/AdoptOpenJDK/openjdk-tests/issues/1162
>> 
>> The test fails on 32bit windows with:
>> 
>> java.lang.IllegalStateException: WB_IncMetaspaceCapacityUntilGC: could not increase capacity until GC due to contention
>> with another thread
>>         at sun.hotspot.WhiteBox.incMetaspaceCapacityUntilGC(Native Method)
>>         at TestCapacityUntilGCWrapAround.main(TestCapacityUntilGCWrapAround.java:51)
>>         at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>>         at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
>>         at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>>         at java.lang.reflect.Method.invoke(Method.java:498)
>>         at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
>>         at java.lang.Thread.run(Thread.java:748)
>> 
>> `TestCapacityUntilGCWrapAround` passes `4GB - 1` to `incMetaspaceCapacityUntilGC()`. It seems to be too big.
>> And also this code seems to want to check the behavior when `_capacity_until_gc` is overflown. White box test would
>> throw ISE when it hapen. So we need to handle it correctly.
>
> Hi Yasumasa,
> 
> sorry, I do not understand what the error is yet and to me the patch does seem wrong in a number of places.
> 
> Both test and whitebox function seem to be doing what they are supposed to do.
> 
> Is this error intermittent or always reproducable? If the latter, since when (since the code seems old)?
> 
> Have the AdoptOpenJDK folks done some pre-analysis?
> 
> ----
> 
> From just looking at the code (cannot build 32bit right now), WB_IncMetaspaceCapacityUntilGC() seems correct to me.
> 
> - in comes a jlong with 4G-1, so FFFF-FFFF
> - it fits into a 32bit size_t so its fine
> - we calculate aligned_inc by aligning the value down somewhat, probably a page: FFFF-F000
> - we feed this value FFFF-F000 as *inc* value to  MetaspaceGC::inc_capacity_until_GC
> - it returns false. We throw an exception.
> 
> In MetaspaceGC::inc_capacity_until_GC:
> 
> bool MetaspaceGC::inc_capacity_until_GC(size_t v, size_t* new_cap_until_GC, size_t* old_cap_until_GC, bool* can_retry) {
>   assert_is_aligned(v, Metaspace::commit_alignment());
> 
>   size_t old_capacity_until_GC = _capacity_until_GC;
>   size_t new_value = old_capacity_until_GC + v;
> 
>   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());
>   }
> 
>   if (new_value > MaxMetaspaceSize) {
>     if (can_retry != NULL) {
>       *can_retry = false;
>     }
>     return false;
>   }
> 
> we enter with size_t v = FFFF-F000
> 
> We calc the new value. Since new value = old value + inc, this will overflow
> 
> We sense the overflow and correct the new value to be max_uintx aligned down by commit alignment.
> 
> Which will be smaller than MaxMetaspaceSize. Test does not set it therefore it is max_uintx.
> 
> So far all good IIUC.
> 
> 
>   if (can_retry != NULL) {
>     *can_retry = true;
>   }
>   size_t prev_value = Atomic::cmpxchg(&_capacity_until_GC, old_capacity_until_GC, new_value);
> 
>   if (old_capacity_until_GC != prev_value) {
>     return false;
>   }
> 
>   if (new_cap_until_GC != NULL) {
>     *new_cap_until_GC = new_value;
>   }
>   if (old_cap_until_GC != NULL) {
>     *old_cap_until_GC = old_capacity_until_GC;
>   }
>   return true;
> 
> From looking at this code, the only way I can see this function returning false would be if a concurrent thread
> modified this threshold. Which should be super rare.
> Hence my initial question about intermittentness. If its reproducable I do not think I understand it yet.
> 
> Thanks, Thomas

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.

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

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