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

[Thomas Stuefe]

On Tue, 13 Oct 2020 10:00:37 GMT, Yasumasa Suenaga <ysuenaga 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

src/hotspot/share/prims/whitebox.cpp line 1745:

> 1743: WB_ENTRY(jlong, WB_IncMetaspaceCapacityUntilGC(JNIEnv* env, jobject wb, jlong inc))
> 1744:   size_t max_size_t = (size_t) -1;
> 1745:   if ((size_t) inc > max_size_t) {

Sorry, I think this is just wrong, the original code is correct (beside the use of -1 instead of using SIZE_MAX which
would have been nicer).

size_t is an unsigned 32bit on 32bit platform so the comparison will be always false. The original coding used a 64bit
jlong for the comparison which is correct. Also, the first comparison for inc < 0 is needed to make the second
comparison work.

test/hotspot/jtreg/gc/metaspace/TestCapacityUntilGCWrapAround.java line 32:

> 30:  * @modules java.base/jdk.internal.misc
> 31:  *          java.management
> 32:  * @requires vm.bits == 32

This test is for a specific 32bit condition, it does not make sense on 64bit.

test/hotspot/jtreg/gc/metaspace/TestCapacityUntilGCWrapAround.java line 46:

> 44:     private static void incMetaspaceCapacityUntilGCTest(WhiteBox wb) {
> 45:         long before = wb.metaspaceCapacityUntilGC();
> 46:         long after = wb.incMetaspaceCapacityUntilGC(100 * MB);

The test makes only sense with 4G increment. AFAIU the counter is supposed to overflow, and cap at MaxMetaspaceSize
instead.

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

Changes requested by stuefe (Reviewer).

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