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

Tue Dec 18 18:23:22 UTC 2012

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.

Do the oops get updated during a GC?

Jon
>> - 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/4e165d42/attachment.htm>