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

Tue Dec 18 18:09:01 UTC 2012

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/57a7a92b/attachment.htm>