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

Coleen Phillimore coleen.phillimore at oracle.com
Tue Dec 18 23:23:22 UTC 2012


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>
>>> 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>
>>> > 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 athttp://cr.openjdk.java.net/~coleenp/7174978_4/
>>> > bug link athttp://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 athttp://cr.openjdk.java.net/~coleenp/7174978_2/
>>> >>>> bug link athttp://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 athttp://cr.openjdk.java.net/~coleenp/7174978/
>>> >>>>> bug link athttp://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/8fdd18b1/attachment.htm>


More information about the hotspot-gc-dev mailing list