<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Coleen,<br>
<br>
It looks good.<br>
Thank you for the comments!<br>
<br>
Thanks,<br>
Serguei<br>
<br>
On 12/14/12 8:05 AM, Coleen Phillimore wrote:<br>
</div>
<blockquote cite="mid:50CB4E5B.7020900@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
I have reworked this change to cleanup backtrace saver C heap
allocated objects (and changed the name) when weak references
are cleaned up, which doesn't have to happen at a full
collection. There is still some performance concerns because
we create these things but a better way of accounting for
Method* in the Java objects is still under design discussions.
Ideas welcome!<br>
<br>
open webrev at <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ecoleenp/7174978_4/">http://cr.openjdk.java.net/~coleenp/7174978_4/</a><br>
bug link at <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://bugs.sun.com/view_bug.do?bug_id=7174978_4">http://bugs.sun.com/view_bug.do?bug_id=7174978_4</a><br>
<br>
One note below.<br>
<br>
On 12/12/2012 12:15 PM, Coleen Phillimore wrote:<br>
</div>
<blockquote cite="mid:50C8BBCA.3090705@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Serguei,<br>
<br>
Thanks for reviewing this. I'm reworking it because in the
pathological case, if there are no full GC's, we wouldn't
reclaim this backtrace C heap storage.<br>
<br>
On 12/11/2012 5:31 PM, <a moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote cite="mid:50C7B437.4080409@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Coleen,<br>
<br>
It looks good in general.<br>
Just some questions below.<br>
<br>
<br>
src/share/vm/classfile/javaClasses.cpp<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
<pre><span class="new"><meta http-equiv="content-type" content="text/html; charset=ISO-8859-1"><span class="new">1339 void java_lang_Throwable::mark_on_stack(oop throwable) {
. . .
</span></span><span class="new">1352 if (method == NULL) return;</span>
Would it be more safe to continue instead of return?<span class="new"><span class="new">
</span></span><span class="new"> 1352 if (method == NULL) continue;</span>
</pre>
</div>
</blockquote>
<br>
This is a short circuit that the function above this also has.
The trace_next_offset will be null in this case. If I change
it, I'd want to change them both.<br>
</blockquote>
<br>
Actually, it would have to be changed to a 'break' because we
create an objArrayOop for the method array and fill it in as the
backtrace is created. Finding a null means there are no more
methods. I prefer to leave the returns in both places. I had a
version where I wrote an iterator, which is "knows" this, but it
didn't feel like saving this many lines was worth the amount of
code the iterator took.<br>
<br>
Thanks,<br>
Coleen<br>
<br>
<blockquote cite="mid:50C8BBCA.3090705@oracle.com" type="cite">
<blockquote cite="mid:50C7B437.4080409@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
src/share/vm/classfile/backtrace.cpp<br>
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
<pre> 63 void Backtrace::do_unloading() {
I guess, this can be called at a safepoint only.
Would it make sense to place a comment or an assert?
</pre>
</div>
</blockquote>
<br>
I added an assert because I assume this. I'm not sure if CMS gc
can unload classes without a safepoint, but this code wants to
be run at a safepoint.<br>
<br>
<blockquote cite="mid:50C7B437.4080409@oracle.com" type="cite">
<div class="moz-cite-prefix">
<pre>I see you already created a new unit test for this:
java/lang/instrument/RedefineMethodInBacktrace.sh</pre>
<br>
</div>
</blockquote>
I think StefanK was going to add this test after I checked in
this fix (or has he already added it?)<br>
<br>
thanks,<br>
Coleen<br>
<br>
<blockquote cite="mid:50C7B437.4080409@oracle.com" type="cite">
<div class="moz-cite-prefix"> Thanks,<br>
Serguei<br>
<br>
<br>
<br>
<br>
<br>
<br>
On 12/10/12 1:15 PM, Coleen Phillimore wrote:<br>
</div>
<blockquote cite="mid:50C650F0.4020405@oracle.com" type="cite">
<br>
I have updated this webrev to include cleanups suggested by
John Rose for the anonymous class fix. Please review
before I add more to this!! <br>
<br>
open webrev at <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ecoleenp/7174978_2/">http://cr.openjdk.java.net/~coleenp/7174978_2/</a>
<br>
bug link at <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://bugs.sun.com/view_bug.do?bug_id=7174978">http://bugs.sun.com/view_bug.do?bug_id=7174978</a>
<br>
<br>
Thanks, <br>
Coleen <br>
<br>
<br>
On 12/05/2012 02:23 PM, Coleen Phillimore wrote: <br>
<blockquote type="cite">Summary: Save the set of backtraces
to use for on stack method walking for redefine classes. <br>
<br>
I also moved metadataOnStackMark class to it's own file
because it's not only used for redefine classes. Some
metadata can be individually deallocated (eg. the Method*
created in the relocator). <br>
<br>
open webrev at <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ecoleenp/7174978/">http://cr.openjdk.java.net/~coleenp/7174978/</a>
<br>
bug link at <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://bugs.sun.com/view_bug.do?bug_id=7174978">http://bugs.sun.com/view_bug.do?bug_id=7174978</a>
<br>
<br>
Ran test that will be added to the jdk/tests in
java/lang/instrument/RedefineMethodInBacktrace.sh (to be
checked in separately). <br>
<br>
thanks, <br>
Coleen <br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>