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

List:       openjdk-serviceability-dev
Subject:    Re: [PING] RFR(s) 8153711: [REDO] JDWP: Memory Leak: GlobalRefs never deleted when processing invoke
From:       Severin Gehwolf <sgehwolf () redhat ! com>
Date:       2016-09-15 8:58:06
Message-ID: 1473929886.2928.3.camel () redhat ! com
[Download RAW message or body]

Hi Serguei,

On Wed, 2016-09-14 at 11:44 -0700, serguei.spitsyn@oracle.com wrote:
> Hi Severin,
> 
> The fix looks good.
> Thank you for persistence in fixing the issue!

Thanks for the review!

> The only suggestion is to refactor the lines 800-815 into a method call.
> Something like deletePoentiallySavedGlobalRefs, similar to  
> deleteGlobalArgumetRefs.

Sure. Updated webrev is here:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.04/

If that looks good, here would be the HG exported version:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/JDK-8153711-jdk9-rc-jdk.export.patch

Cheers,
Severin

> Thanks,
> Serguei
> 
> 
> 
> On 9/14/16 09:34, Severin Gehwolf wrote:
> > 
> > Anyone?
> > 
> > On Fri, 2016-09-09 at 18:27 +0200, 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]