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

Jon Masamitsu jon.masamitsu at oracle.com
Wed Dec 19 16:23:12 UTC 2012 


On 12/18/2012 11:55 AM, Coleen Phillimore wrote:
>
> 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?

The scripts don't pick out the Reference processing time as far as  I know.

Regarding which tests, since the concern is that this fix might place
a burden on all applications, I think the refworkload with reference_server
would be good.   Point me at a build and I'll run it and look for GC
performance regressions.

Jon

>
> 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
>>>> >>>>
>>>> >>>
>>>> >>
>>>> >
>>>> >
>>>> >
>>>> >
>>>> >
>>>
>
>

More information about the hotspot-gc-dev mailing list