[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(s) 8153711: [REDO] JDWP: Memory Leak: GlobalRefs never deleted when processing invokeMethod 
From:       "Daniel D. Daugherty" <daniel.daugherty () oracle ! com>
Date:       2016-09-15 14:25:22
Message-ID: a5d32034-7842-335d-a9ee-1a31fd1deab6 () oracle ! com
[Download RAW message or body]

On 9/15/16 8:18 AM, Severin Gehwolf wrote:
> On Thu, 2016-09-15 at 08:15 -0600, Daniel D. Daugherty wrote:
>> Dmitry,
>>
>> This fix needs to be run through the entire JPDA test stack
>> before it is pushed. Don't know if we still have test definitions
>> to support that style of run anymore so it might be easier to
>> run it through the equivalent of a JDK9-hs nightly.
> Hmm, it's been pushed already:
> http://hg.openjdk.java.net/jdk9/hs/jdk/rev/4c843eb35b8a

Sigh... I haven't gotten that far in clearing my email queue
this AM. Hopefully we don't see failures like we did with
the earlier rounds.

Dan


>
> Cheers,
> Severin
>
>> Dan
>>
>> On 9/14/16 11:50 AM, Dmitry Samersoff wrote:
>>> Severin,
>>>
>>> The fix looks good for me.
>>>
>>> I'll sponsor the push, but please wait for Serguei.
>>>
>>> -Dmitry
>>>
>>>
>>> On 2016-09-09 19:27, Severin Gehwolf wrote:
>>>> Hi,
>>>>
>>>> Could I please get a review of the this 4th version of this fix:
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8153711
>>>> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.03/
>>>>
>>>> It fixes a memory leak problem in the debugger as shown by the new
>>>> regression test.
>>>>
>>>> A bit of history to this new patch. The first version[1] of this patch,
>>>> pushed as fix for JDK-4858370, caused regressions in
>>>> InterfaceMethodsTest, InvokeTest and OomDebugTest (part of the fix).
>>>> The cause was not holding the invoker lock when clearing the
>>>> references. A subsequent version[2] caused deadlocks, because we were
>>>> holding the invoker lock while invoking in invoker_doInvoke().
>>>>
>>>> Finally, a third version[3] caused NPE's and asserts on Solaris. The
>>>> reason for them seems to be clearing the request->clazz and request-
>>>>> instance members *after* sending back the reply to the client. My
>>>> hypothesis is that it maybe related to the sequence of monitor_exit-
>>>>> perform IO->monitor_enter->toss references. Perhaps there is a
>>>> schedule where the response has been sent back, the next invoke starts
>>>> for the same app thread and it is just far enough along so that the
>>>> tossing of the references becomes observable by the next request.
>>>> Unfortunately, I don't have proof for this.
>>>>
>>>> However, testing showed that tossing request->clazz and request-
>>>>> instance members before the IO operation prevents this assertion from
>>>> being triggered. Thus, I'm now clearing global references - the ones we
>>>> can clear before sending back the response to the client - in the same
>>>> block while still holding the invoker and event handler locks as the
>>>> rest of the operations being done in completeInvokeRequest. Once the
>>>> response has been sent to the client there are still two global
>>>> references to clear: The one for the return value and a possible
>>>> exception which might have occurred. Since they are required for
>>>> sending the response to the client we do this after it's been sent.
>>>>
>>>> I think not holding the invoker lock while invoking is still a problem,
>>>> but that's being tracked in JDK-8156498.
>>>>
>>>> Testing done:
>>>>
>>>> - com/sun/jdi test-set. No regressions.
>>>>     New OomDebugTest passes. Fails before.
>>>> - I haven't observed hangs or regressions in 1000 runs on
>>>>     com/sun/jdi/InvokeTest.java
>>>>     com/sun/jdi/InvokeHangTest.java
>>>>     com/sun/jdi/OomDebugTest.java on Linux x86_64 release/fastdebug
>>>> - I haven't seen asserts being triggered on Solaris x86_64
>>>>     fastdebug of 100 iterations of:
>>>>     com/sun/jdi/InvokeTest.java
>>>>     com/sun/jdi/InvokeHangTest.java
>>>>     com/sun/jdi/OomDebugTest.java
>>>>
>>>> I believe I need a sponsor who can push this fix through JPRT once
>>>> reviewed.
>>>>
>>>> Thoughts?
>>>>
>>>> Thanks,
>>>> Severin
>>>>
>>>> [1] http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/277d7584fa03
>>>> [2] http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.01/
>>>> [3] http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.02/
>>>>

[prev in list] [next in list] [prev in thread] [next in thread]