URGENT RFR 8153673 [BACKOUT] JDWP: Memory Leak: GlobalRefs never deleted when processing invokeMethod command

[serguei.spitsyn at oracle.com]

On 4/6/16 16:51, Daniel D. Daugherty wrote:
> On 4/6/16 5:45 PM, serguei.spitsyn at oracle.com wrote:
>> Hi Dan,
>>
>> The fix looks right.
>
> Thanks!
>
>
>> Thank you for taking care about the hs-rt stability!
>
> You are very welcome.
>
>
>> It makes sense to back the fix of the 4858370 out at least because
>> it introduced new test that started failing intermittently.
>>
>> The following 3 bugs have similar manifestation and related to the 
>> remote invocation
>> of a method or a constructor in the target VM:
>> https://bugs.openjdk.java.net/browse/JDK-8152586
>> https://bugs.openjdk.java.net/browse/JDK-8152686
>> https://bugs.openjdk.java.net/browse/JDK-8152985
>
> I wasn't sure that JDK-8152586 was part of the mix because I
> thought it was first spotted before JDK-4858370 was pushed...
> but I based that guess on your notes...
>
> I suspect that the cleanup of the global refs is happening
> too early and some things that were relying on those globals
> refs are now unhappy... but it has been a long time since I've
> been in that code...

I meditated on the JDK-4858370 fix for some time but did not find anything
specifically wrong and was of guesses.
One issue with this fix is that it is impossible to track the issue back.
Earlier jprt builds fail with OOM on new test as it checks that 
GlobalRefs are properly deleted.

Thanks,
Serguei

>
> Dan
>
>
>>
>>
>> I'm still in process of isolation of the JDK-8152586 regression.
>> I took a wrong path at some point and need to redo some of my steps.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 4/6/16 16:34, Daniel D. Daugherty wrote:
>>> Resending with Jesper and Serguei on directly...
>>>
>>> Greetings,
>>>
>>> It appears that the fix for this bug:
>>>
>>>     JDK-4858370 JDWP: Memory Leak: GlobalRefs never deleted when 
>>> processing
>>>                 invokeMethod command
>>> https://bugs.openjdk.java.net/browse/JDK-4858370
>>>
>>> has been causing intermittent test failures in the JDK9-hs-rt nightly.
>>>
>>>
>>> ************************************************************
>>> I need reviews from Jesper Wilhelmsson and Serguei Spitsyn.
>>> ************************************************************
>>>
>>>
>>> This bug is being using to backout JDK-4858370:
>>>
>>>     JDK-8153673 [BACKOUT] JDWP: Memory Leak: GlobalRefs never 
>>> deleted when
>>>                 processing invokeMethod command
>>> https://bugs.openjdk.java.net/browse/JDK-8153673
>>>
>>> Here is the backout webrev URL:
>>>
>>> http://cr.openjdk.java.net/~dcubed/8153673-webrev/0-jdk9-hs-rt-jdk/
>>>
>>> Here are the changeset link for JDK-4858370:
>>>
>>> http://hg.openjdk.java.net/jdk9/hs-rt/jdk/rev/277d7584fa03
>>>
>>> Jesper and Serguei, thanks, in advance, for the reviews!
>>>
>>> Gory details below...
>>>
>>> Dan
>>>
>>>
>>>
>>> Here's my jdk repo sanity check note:
>>>
>>>  Daniel Daugherty added a comment - 1 hour ago
>>> Sanity checks for backing out this fix and its test:
>>>
>>> $ hg log -r 277d7584fa03
>>> changeset: 13880:277d7584fa03
>>> parent: 13877:645a9be6eddb
>>> user: sgehwolf
>>> date: Mon Mar 21 11:24:09 2016 +0100
>>> summary: 4858370: JDWP: Memory Leak: GlobalRefs never deleted when 
>>> processing invokeMethod command
>>>
>>> $ hg status
>>> M src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
>>> R test/com/sun/jdi/OomDebugTest.java
>>> ? files.list
>>>
>>> $ hg diff -r 13877 `cat files.list `
>>> <empty output>
>>>
>>> $ hg diff src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
>>> diff -r 96b1cfa80016 src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
>>> --- a/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c Sat Apr 02 
>>> 05:30:48 2016 +0200
>>> +++ b/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c Wed Apr 06 
>>> 14:57:33 2016 -0700
>>> @@ -209,47 +209,6 @@ createGlobalRefs(JNIEnv *env, InvokeRequ
>>>      }
>>>
>>>      return error;
>>> -}
>>> -
>>> -/*
>>> - * Delete global references from the request which got put there 
>>> before a
>>> - * invoke request was carried out. See fillInvokeRequest() and 
>>> invoker invoke*()
>>> - * impls.
>>> - */
>>> -static void
>>> -deleteGlobalRefs(JNIEnv *env, InvokeRequest *request)
>>> -{
>>> - void *cursor;
>>> - jint argIndex = 0;
>>> - jvalue *argument = request->arguments;
>>> - jbyte argumentTag = firstArgumentTypeTag(request->methodSignature, 
>>> &cursor);
>>> -
>>> - if (request->clazz != NULL) {
>>> - tossGlobalRef(env, &(request->clazz));
>>> - }
>>> - if (request->instance != NULL) {
>>> - tossGlobalRef(env, &(request->instance));
>>> - }
>>> - /* Delete global argument references */
>>> - while (argIndex < request->argumentCount) {
>>> - if ((argumentTag == JDWP_TAG(OBJECT)) ||
>>> - (argumentTag == JDWP_TAG(ARRAY))) {
>>> - if (argument->l != NULL) {
>>> - tossGlobalRef(env, &(argument->l));
>>> - }
>>> - }
>>> - argument++;
>>> - argIndex++;
>>> - argumentTag = nextArgumentTypeTag(&cursor);
>>> - }
>>> - /* Delete potentially saved return values */
>>> - if ((request->invokeType == INVOKE_CONSTRUCTOR) ||
>>> - (returnTypeTag(request->methodSignature) == JDWP_TAG(OBJECT)) ||
>>> - (returnTypeTag(request->methodSignature) == JDWP_TAG(ARRAY))) {
>>> - if (request->returnValue.l != NULL) {
>>> - tossGlobalRef(env, &(request->returnValue.l));
>>> - }
>>> - }
>>>  }
>>>
>>>  static jvmtiError
>>> @@ -777,13 +736,6 @@ invoker_completeInvokeRequest(jthread th
>>>          (void)outStream_writeObjectRef(env, &out, exc);
>>>          outStream_sendReply(&out);
>>>      }
>>> -
>>> - /*
>>> - * At this time, there's no need to retain global references on
>>> - * arguments since the reply is processed. No one will deal with
>>> - * this request ID anymore, so we must call deleteGlobalRefs().
>>> - */
>>> - deleteGlobalRefs(env, request);
>>>  }
>>>
>>>  jboolean
>>>
>>> $ hg diff -r 13877 src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
>>> <empty output>
>>>
>>
>