[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: code review request for Redefine and Retransform Classes memory leak (7121600, 7122253)
From: serguei.spitsyn () oracle ! com (serguei ! spitsyn at oracle ! com)
Date: 2011-12-28 23:13:36
Message-ID: 4EFBA2A0.2000601 () oracle ! com
[Download RAW message or body]
On 12/21/11 2:41 PM, Daniel D. Daugherty wrote:
> Karen,
>
> Thanks for the quick review! Replies in-lined below...
>
>
> On 12/21/11 2:47 PM, Karen Kinnear wrote:
> > Dan,
> >
> > All versions of hotspot changes look good.
>
> Thanks! Are you OK if I don't wait for someone from the new
> Serviceability team on this review? Serguei has already left
> for the holidays... and I told him to ignore any e-mails from
> me :-)
Dan,
I honestly ignored all emails from you while was on vacation. :)
But reviewed your fixes upon return anyway.
The fixes look good.
I can't give you any new comment.
Great work!
Thanks,
Serguei
> > For the JDK and test side:
> > 1) minor: JPLISAgent.c - new lines 1210-1212 might just be an } else {
>
> Nice catch! This line:
>
> 1212 if (!errorOccurred) {
>
> should change back to:
>
> 1212 else {
>
> I missed that when I was backing out some more complicated
> stuff that I gave up on.
>
> Fixed in all three versions.
>
>
> > 2) new lines 1311-1312
> > Why do you break if an error occurred?
>
> Because I was trying to match the existing style
> too much. The for-loop from lines 1235-1276 does
> that: if I have an error, then break...
>
>
> > If there is a reason to stop freeing memory, would that also
> > apply if
> > you already had had an error? So you would want to check a new
> > cleanuperrorOccurred
> > regardless of pre-existing error?
> > But I suspect leaving out the break would make more sense.
>
> For this block:
>
> 1304 /*
> 1305 * Only check for error if we didn't
> already have one
> 1306 * so we don't overwrite errorOccurred.
> 1307 */
> 1308 if (!errorOccurred) {
> 1309 errorOccurred =
> checkForThrowable(jnienv);
> 1310 jplis_assert(!errorOccurred);
> 1311 if (errorOccurred) {
> 1312 break;
> 1313 }
> 1314 }
>
> I'm going to delete lines 1311-1313 so we'll just try to keep
> freeing as many of the memory arrays as we can...
>
> Fixed in all three versions.
>
> On a related note, jplis_assert() appears to be enabled even
> in product bits. Which means if one of the many jplis_assert()
> calls fails, then the agent terminates. I don't know the history
> behind it being enabled in all bits, but I figure that's a
> different issue for the Serviceability team to mull on...
>
> Thanks again for the review.
>
> Dan
>
>
>
>
> > thanks,
> > Karen
> >
> > On Dec 21, 2011, at 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
> > >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20111228/fe8af3af/attachment-0001.html \
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic