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

Tue Dec 18 19:55:59 UTC 2012

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.

I ran specjvm to see if there was an impact overall (at least for 
specjvm98).   Which test should I run this with and does your GC magic 
comparison script compare these numbers?

Thanks,
Coleen

>
> 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/9380ef36/attachment.htm>