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.
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/
