On Wed, 2016-09-14 at 20:50 +0300, Dmitry Samersoff wrote: > Severin, > > The fix looks good for me. > > I'll sponsor the push, but please wait for Serguei.
Thanks, Dmitry. Cheers, Severin > -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/we > > brev.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/ > > > >
