Request for review: 7174978: NPG: Fix bactrace builder for class redefinition

Srinivas Ramakrishna ysr1729 at gmail.com
Wed Dec 19 00:43:35 UTC 2012 

Coleen, I'll look at the webrev. A quick browse seemed to suggest that this
may not be the "right" way to do it
(or at least not consistent with established structure for weak root
processing),
even though it might work. I'll email my review later tonight. If possible,
please wait for my review --
promise to do it tonight.

-- ramki

On Dec 18, 2012, at 15:23, Coleen Phillimore <coleen.phillimore at oracle.com>
wrote:


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.
Thanks,
Coleen

On 12/18/2012 01:46 PM, Jon Masamitsu wrote:

Coleen,

Changes look correct.

You can run with -XX:+PrintReferenceGC to see if
there is an impact on the Reference processing time.

Jon


On 12/18/12 10:09, Coleen Phillimore wrote:


Thanks Jon.  I'm not on hotspot-gc so I didn't see it.

On 12/18/2012 11:02 AM, Jon Masamitsu wrote:

I started  reviewing and had these questions.
Did I miss your response?

Jon

-------- Original Message --------  Subject: Re: Fwd: Re: Request for
review: 7174978: NPG: Fix bactrace builder for class redefinition  Date: Mon,
17 Dec 2012 12:50:46 -0800  From: Jon Masamitsu
<jon.masamitsu at oracle.com><jon.masamitsu at oracle.com>  Organization:
Oracle Corporation  To: hotspot-gc-dev at openjdk.java.net

Coleen,

Is this what this change is  doing?

- Creating a list of oops XX (BacktraceSaver::add()).


Yes, they are jweak oops because they are not normal GC roots.

- 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()).


Yes.

Thanks,
Coleen

 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 <coleen.phillimore at oracle.com> <coleen.phillimore at oracle.com>
> Reply-To: coleen.phillimore at oracle.com
> Organization:     Oracle Corporation
> To: hotspot-runtime-dev at openjdk.java.net
>
>
>
>
> 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 http://cr.openjdk.java.net/~coleenp/7174978_4/
> bug link at http://bugs.sun.com/view_bug.do?bug_id=7174978_4
>
> 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, serguei.spitsyn at oracle.com 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 http://cr.openjdk.java.net/~coleenp/7174978_2/
>>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=7174978
>>>>
>>>> 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 http://cr.openjdk.java.net/~coleenp/7174978/
>>>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=7174978
>>>>>
>>>>> Ran test that will be added to the jdk/tests in
>>>>> java/lang/instrument/RedefineMethodInBacktrace.sh (to be checked
>>>>> in separately).
>>>>>
>>>>> thanks,
>>>>> Coleen
>>>>
>>>
>>
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20121218/25fef8f4/attachment.htm>

More information about the hotspot-gc-dev mailing list