Fwd: Re: Fwd: Re: Request for review: 7174978: NPG: Fix bactrace builder for class redefinition
Jon Masamitsu
jon.masamitsu at oracle.com
Tue Dec 18 18:46:17 UTC 2012
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/7fbbd4fd/attachment.htm>
More information about the hotspot-gc-dev
mailing list