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

[Jon Masamitsu]

Coleen,

Is this what this change is  doing?

- Creating a list of oops XX (BacktraceSaver::add()).
- 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()).

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