<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
I should point out that I borrowed this idea from the way that
redefine classes used to process previous versions of methods -
PreviousVersionWalker.<br>
<br>
thanks,<br>
Coleen<br>
<br>
<div class="moz-cite-prefix">On 12/18/2012 07:43 PM, Srinivas
Ramakrishna wrote:<br>
</div>
<blockquote
cite="mid:CABzyjym_iEjeFpwfd_e3r7hhq=iyEe-DT6NQt7J+ONEbXiRt-g@mail.gmail.com"
type="cite">
<div dir="auto">Coleen, I'll look at the webrev. A quick browse
seemed to suggest that this may not be the "right" way to do it<br>
(or at least not consistent with established structure for weak
root processing),<br>
even though it might work. I'll email my review later tonight.
If possible, please wait for my review --<br>
promise to do it tonight.<br>
<br>
-- ramki<br>
<br>
<div>On Dec 18, 2012, at 15:23, Coleen Phillimore <<a
moz-do-not-send="true"
href="mailto:coleen.phillimore@oracle.com" target="_blank">coleen.phillimore@oracle.com</a>>
wrote:<br>
<br>
</div>
<blockquote type="cite">
<div> <br>
The PrintReferenceGC indicates that we add about 0.0000010
secs to the JNI Weak processing for each GC. Is that too
much? If so, we should look at the performance (and
fragility) of all the metadata marking in a separate bug.
We might need a GC-centric solution.<br>
Thanks,<br>
Coleen<br>
<br>
<div>On 12/18/2012 01:46 PM, Jon Masamitsu wrote:<br>
</div>
<blockquote type="cite"> Coleen,<br>
<br>
Changes look correct.<br>
<br>
You can run with -XX:+PrintReferenceGC to see if<br>
there is an impact on the Reference processing time.<br>
<br>
Jon<br>
<br>
<br>
On 12/18/12 10:09, Coleen Phillimore wrote:
<blockquote type="cite"> <br>
Thanks Jon. I'm not on hotspot-gc so I didn't see it.<br>
<br>
<div>On 12/18/2012 11:02 AM, Jon Masamitsu wrote:<br>
</div>
<blockquote type="cite"> I started reviewing and had
these questions.<br>
Did I miss your response?<br>
<br>
Jon<br>
<br>
-------- Original Message --------
<table cellpadding="0" cellspacing="0" border="0">
<tbody>
<tr>
<th align="RIGHT" nowrap="nowrap"
valign="BASELINE">Subject: </th>
<td>Re: Fwd: Re: Request for review: 7174978:
NPG: Fix bactrace builder for class
redefinition</td>
</tr>
<tr>
<th align="RIGHT" nowrap="nowrap"
valign="BASELINE">Date: </th>
<td>Mon, 17 Dec 2012 12:50:46 -0800</td>
</tr>
<tr>
<th align="RIGHT" nowrap="nowrap"
valign="BASELINE">From: </th>
<td>Jon Masamitsu <a moz-do-not-send="true"
href="mailto:jon.masamitsu@oracle.com"
target="_blank"><jon.masamitsu@oracle.com></a></td>
</tr>
<tr>
<th align="RIGHT" nowrap="nowrap"
valign="BASELINE">Organization: </th>
<td>Oracle Corporation</td>
</tr>
<tr>
<th align="RIGHT" nowrap="nowrap"
valign="BASELINE">To: </th>
<td><a moz-do-not-send="true"
href="mailto:hotspot-gc-dev@openjdk.java.net"
target="_blank">hotspot-gc-dev@openjdk.java.net</a></td>
</tr>
</tbody>
</table>
<br>
<br>
<pre>Coleen,
Is this what this change is doing?
- Creating a list of oops XX (BacktraceSaver::add()).</pre>
</blockquote>
<br>
Yes, they are jweak oops because they are not normal GC
roots.<br>
<blockquote type="cite">
<pre>- Walking XX to find which oops point to live objects and keeping
those objects alive (BacktraceSaver::mark_on_stack())
- Walking the list to delete items in the list for dead objects
(BacktraceSaver::cleanup_backtrace_list()).</pre>
</blockquote>
<br>
Yes.<br>
<br>
Thanks,<br>
Coleen<br>
<br>
<blockquote type="cite">
<pre>Jon
On 12/17/2012 5:48 AM, Coleen Phillimore wrote:
>
> Hi, Can someone from the GC group review this? I added code to
> reference processor to clean up backtrace marker objects (that's a
> better name, isn't it?).
> Thanks,
> Coleen
>
>
> -------- Original Message --------
> Subject: Re: Request for review: 7174978: NPG: Fix bactrace
> builder for class redefinition
> Date: Fri, 14 Dec 2012 11:05:47 -0500
> From: Coleen Phillimore <a moz-do-not-send="true" href="mailto:coleen.phillimore@oracle.com" target="_blank"><coleen.phillimore@oracle.com></a>
> Reply-To: <a moz-do-not-send="true" href="mailto:coleen.phillimore@oracle.com" target="_blank">coleen.phillimore@oracle.com</a>
> Organization: Oracle Corporation
> To: <a moz-do-not-send="true" href="mailto:hotspot-runtime-dev@openjdk.java.net" target="_blank">hotspot-runtime-dev@openjdk.java.net</a>
>
>
>
>
> 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!
>
> open webrev at <a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Ecoleenp/7174978_4/" target="_blank">http://cr.openjdk.java.net/~coleenp/7174978_4/</a>
> bug link at <a moz-do-not-send="true" href="http://bugs.sun.com/view_bug.do?bug_id=7174978_4" target="_blank">http://bugs.sun.com/view_bug.do?bug_id=7174978_4</a>
>
> One note below.
>
> On 12/12/2012 12:15 PM, Coleen Phillimore wrote:
>>
>> Serguei,
>>
>> 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.
>>
>> On 12/11/2012 5:31 PM, <a moz-do-not-send="true" href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a> wrote:
>>> Coleen,
>>>
>>> It looks good in general.
>>> Just some questions below.
>>>
>>>
>>> src/share/vm/classfile/javaClasses.cpp
>>>
>>> 1339 void java_lang_Throwable::mark_on_stack(oop throwable) {
>>> . . .
>>> 1352 if (method == NULL) return;
>>>
>>> Would it be more safe to continue instead of return?
>>> 1352 if (method == NULL) continue;
>>
>> 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.
>
> 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.
>
> Thanks,
> Coleen
>
>>>
>>> src/share/vm/classfile/backtrace.cpp
>>> 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?
>>
>> 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.
>>
>>> I see you already created a new unit test for this:
>>> java/lang/instrument/RedefineMethodInBacktrace.sh
>>>
>> I think StefanK was going to add this test after I checked in this
>> fix (or has he already added it?)
>>
>> thanks,
>> Coleen
>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>
>>>
>>>
>>>
>>> On 12/10/12 1:15 PM, Coleen Phillimore wrote:
>>>>
>>>> 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!!
>>>>
>>>> open webrev at <a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Ecoleenp/7174978_2/" target="_blank">http://cr.openjdk.java.net/~coleenp/7174978_2/</a>
>>>> bug link at <a moz-do-not-send="true" href="http://bugs.sun.com/view_bug.do?bug_id=7174978" target="_blank">http://bugs.sun.com/view_bug.do?bug_id=7174978</a>
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>>
>>>> On 12/05/2012 02:23 PM, Coleen Phillimore wrote:
>>>>> Summary: Save the set of backtraces to use for on stack method
>>>>> walking for redefine classes.
>>>>>
>>>>> 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).
>>>>>
>>>>> open webrev at <a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Ecoleenp/7174978/" target="_blank">http://cr.openjdk.java.net/~coleenp/7174978/</a>
>>>>> bug link at <a moz-do-not-send="true" href="http://bugs.sun.com/view_bug.do?bug_id=7174978" target="_blank">http://bugs.sun.com/view_bug.do?bug_id=7174978</a>
>>>>>
>>>>> Ran test that will be added to the jdk/tests in
>>>>> java/lang/instrument/RedefineMethodInBacktrace.sh (to be checked
>>>>> in separately).
>>>>>
>>>>> thanks,
>>>>> Coleen
>>>>
>>>
>>
>
>
>
>
>
</pre>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</div>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>