Sure, that's okay then.
Coleen
On 12/21/2011 4:25 PM, Daniel D. Daugherty wrote:
Coleen,
Thanks for the quick review!
I don't think it would be too hard to refactor that code, but
I would prefer not to for a couple of reasons:
- the JDK6u29 version will likely be pushed as an escalation fix
and refactoring is frowned upon unless absolutely necessary
- for ease of determining correctness with all three versions,
they should remain as similar as possible
Are you OK with this resolution? Please let me know.
Dan
On 12/21/11 2:06 PM, Coleen Phillimore wrote:
Dan, I reviewed one of the hotspot versions and it looks good to me.
I have one request that the code in parseClassFile in the
conditional JvmtiExport::should_post_class_file_load_hook() { ... }
be made into it's own function because it's not the straight line
case and is getting to be a larger block of code. If it's not hard
to do.
We still want to know how you found it... very impressive!
Thanks,
Coleen
On 12/21/2011 2:09 PM, Daniel D. Daugherty wrote:
Greetings,
This is a code review request for "day one" memory leaks in
the java.lang.instrument.Instrumentation.redefineClasses()
and JVM/TI RetransformClasses() APIs.
Since one leak is in the JDK and the other leak is in the
VM, I'm sending out separate webrevs for each repo. I'm also
attacking these leaks in three releases in parallel so there
is a pair of webrevs for: JDK6u29, JDK7u4 and JDK8. Yes, I'm
trying to get this all done before Christmas!
Here are the bug links:
7121600 2/3 Instrumentation.redefineClasses() leaks class bytes
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7121600
http://monaco.sfbay.sun.com/detail.jsp?cr=7121600
7122253 2/3 Instrumentation.retransformClasses() leaks class bytes
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7121600
http://monaco.sfbay.sun.com/detail.jsp?cr=7122253
I don't know why the bugs.sun.com links aren't showing up yet...
I think it is best to review the JDK7u4/HSX-23-B06 webrevs first.
Here are the webrevs for the JDK6u29/HSX-20 version of the fixes:
http://cr.openjdk.java.net/~dcubed/7121600-webrev/1-jdk6u29/
http://cr.openjdk.java.net/~dcubed/7122253-webrev/0-hsx20/
I expect the OpenJDK6 version of the fixes to very similar if not
identical to the JDK6u29 version. I haven't been tracking the
OpenJDK6 project as closely as I used to so I don't know whether
these fixes should go back there also.
Here are the webrevs for the JDK7u4/HSX-23-B06 version of the fixes:
http://cr.openjdk.java.net/~dcubed/7121600-webrev/1-jdk7u4/
http://cr.openjdk.java.net/~dcubed/7122253-webrev/0-hsx23-b06/
The JDK7u4 JLI code has some other, unrelated fixes relative to
the JDK6u29 JLI code. That required a very minor change in my fix
to JPLISAgent.c to insulate against unexpected JVM/TI phase values
in a slightly different way than the original JDK7u4 code.
Also, JDK7u4 was updated to the HSX-23-B08 snapshot last night, but
I baselined and tested the fix against HSX-23-B06 so I'm sending out
the webrev for what I actually used.
Here are the webrevs for the JDK8/HSX-23-B08 version of the fixes:
http://cr.openjdk.java.net/~dcubed/7121600-webrev/1-jdk8/
http://cr.openjdk.java.net/~dcubed/7122253-webrev/0-hsx23-b08/
The JDK7u4 and JDK8 versions of the fix for 7121600 are identical.
The HSX-23-B06 and HSX-23-B08 versions of the fix for 7122253 are
also identical.
I've run the following test suites:
- VM/NSK jvmti, VM/NSK jvmti_unit
- VM/NSK jdwp
- SDK/JDK com/sun/jdi, SDK/JDK closed/com/sun/jdi, VM/NSK jdi
- SDK/JDK java/lang/instrument
- VM/NSK hprof, VM/NSK jdb, VM/NSK sajdi
- VM/NSK heapdump
- SDK/JDK misc_attach, SDK/JDK misc_jvmstat, SDK/JDK misc_tools
on the following configs:
- {Client VM, Server VM} x {product, fastdebug} x {-Xmixed, -Xcomp}
- Solaris X86 JDK6u29/HSX-20 (done - no regressions)
- Solaris X86 JDK7u4/HSX-23-B06 (done - no regressions)
- WinXP JDK7u4/HSX-23-B06 (in process, no regressions so far)
- Solaris X86 JDK8/HSX-23-B08 (just started)
- WinXP JDK8/HSX-23-B08 (not yet started)
Thanks, in advance, for any feedback...
Dan
