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

[Coleen Phillimore]

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 at http://cr.openjdk.java.net/~coleenp/7174978_4/
bug link at http://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 at http://cr.openjdk.java.net/~coleenp/7174978_2/
>>> bug link at http://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 at http://cr.openjdk.java.net/~coleenp/7174978/
>>>> bug link at http://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/20121217/57f22c2f/attachment.htm>