Request for review: 7174978: NPG: Fix bactrace builder for class redefinition
Coleen Phillimore
coleen.phillimore at oracle.com
Wed Dec 19 01:06:25 UTC 2012
I should point out that I borrowed this idea from the way that redefine
classes used to process previous versions of methods -
PreviousVersionWalker.
thanks,
Coleen
On 12/18/2012 07:43 PM, Srinivas Ramakrishna wrote:
> Coleen, I'll look at the webrev. A quick browse seemed to suggest that
> this may not be the "right" way to do it
> (or at least not consistent with established structure for weak root
> processing),
> even though it might work. I'll email my review later tonight. If
> possible, please wait for my review --
> promise to do it tonight.
>
> -- ramki
>
> On Dec 18, 2012, at 15:23, Coleen Phillimore
> <coleen.phillimore at oracle.com <mailto:coleen.phillimore at oracle.com>>
> wrote:
>
>>
>> The PrintReferenceGC indicates that we add about 0.0000010 secs to
>> the JNI Weak processing for each GC. Is that too much? If so, we
>> should look at the performance (and fragility) of all the metadata
>> marking in a separate bug. We might need a GC-centric solution.
>> Thanks,
>> Coleen
>>
>> 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.
>>>
>>> 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>
>>>>> <mailto:jon.masamitsu at oracle.com>
>>>>> Organization: Oracle Corporation
>>>>> To: hotspot-gc-dev at openjdk.java.net
>>>>> <mailto: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> <mailto:coleen.phillimore at oracle.com>
>>>>> > Reply-To:coleen.phillimore at oracle.com <mailto:coleen.phillimore at oracle.com>
>>>>> > Organization: Oracle Corporation
>>>>> > To:hotspot-runtime-dev at openjdk.java.net <mailto: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/ <http://cr.openjdk.java.net/%7Ecoleenp/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 <mailto: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/ <http://cr.openjdk.java.net/%7Ecoleenp/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/ <http://cr.openjdk.java.net/%7Ecoleenp/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/e161e0aa/attachment.htm>
More information about the hotspot-gc-dev
mailing list