<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<br>
On 12/18/12 10:09, Coleen Phillimore wrote:
<blockquote cite="mid:50D0B13D.5080205@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<br>
Thanks Jon. I'm not on hotspot-gc so I didn't see it.<br>
<br>
<div class="moz-cite-prefix">On 12/18/2012 11:02 AM, Jon Masamitsu
wrote:<br>
</div>
<blockquote cite="mid:50D0939A.7040409@oracle.com" type="cite">
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
I started reviewing and had these questions.<br>
Did I miss your response?<br>
<br>
Jon<br>
<br>
-------- Original Message --------
<table class="moz-email-headers-table" border="0"
cellpadding="0" cellspacing="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"
class="moz-txt-link-rfc2396E"
href="mailto:jon.masamitsu@oracle.com"><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"
class="moz-txt-link-abbreviated"
href="mailto:hotspot-gc-dev@openjdk.java.net">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>
<br>
Do the oops get updated during a GC?<br>
<br>
Jon<br>
<blockquote cite="mid:50D0B13D.5080205@oracle.com" type="cite">
<blockquote cite="mid:50D0939A.7040409@oracle.com" 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 cite="mid:50D0939A.7040409@oracle.com" 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" class="moz-txt-link-rfc2396E" href="mailto:coleen.phillimore@oracle.com"><coleen.phillimore@oracle.com></a>
> Reply-To: <a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:coleen.phillimore@oracle.com">coleen.phillimore@oracle.com</a>
> Organization: Oracle Corporation
> To: <a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:hotspot-runtime-dev@openjdk.java.net">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" class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ecoleenp/7174978_4/">http://cr.openjdk.java.net/~coleenp/7174978_4/</a>
> 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>
>
> 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" class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">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" class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ecoleenp/7174978_2/">http://cr.openjdk.java.net/~coleenp/7174978_2/</a>
>>>> 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>
>>>>
>>>> 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" class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Ecoleenp/7174978/">http://cr.openjdk.java.net/~coleenp/7174978/</a>
>>>>> 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>
>>>>>
>>>>> 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>
</body>
</html>