review request for CR 6361589

Tom Rodriguez Thomas.Rodriguez at Sun.COM
Fri Dec 11 08:09:30 PST 2009


Looks good.

tom

On Dec 10, 2009, at 8:10 PM, Yumin Qi wrote:

> Thanks.
> 
> I modified the code and submitted again. Please take a look again, will commit if it is OK.
> 
> Thanks
> Yumin
> 
> Coleen Phillimore wrote:
>> 
>> Yumin,
>> I've already reviewed a previous version and this still looks great to me.  I hope this makes diagnosing crashes during gc much better, rather than having to decode the thread manually as you've been doing for years.
>> Thanks everyone for the additional comments.  The later suggested changes are really good.
>> 
>> Coleen
>> 
>> Yumin Qi wrote:
>>> Thanks for you all the good suggestions:
>>> 
>>> 1) update the hierarchy comments --- we miss some there
>>> 2) Use c'tor-d'tor idiom for remembering JavaThread  in  GC thread
>>> 3) Add one more STEP (135)
>>> 4) update vmStructs so the SA can check this value.
>>> 
>>> Thanks
>>> Yumin
>>> 
>>> 
>>> 
>>> David Holmes - Sun Microsystems wrote:
>>>> Hi Yumin,
>>>> 
>>>> Can you update the thread hierarchy comments in thread.hpp:
>>>> 
>>>>  49 // Class hierarchy
>>>>  50 // - Thread
>>>>  51 //   - VMThread
>>>>  52 //   - JavaThread
>>>>  53 //   - WatcherThread
>>>> 
>>>> This seems incomplete anyway so it would be good to update it.
>>>> 
>>>> I would also find it useful to add more comments to the NamedThread class explaining the role of this new field. It really has nothing to do with being a "named thread" - but then NamedThread should really have been called something more general. Pity we can't rename it - can we? :)
>>>> 
>>>> In vmError.cpp:
>>>> 
>>>> 506      // printing Java thread stack trace if it is involved in GC crash
>>>> 507
>>>> 508      if (_verbose && (_thread->is_Named_thread())) {
>>>> 
>>>> Can/Should this be a new STEP?
>>>> 
>>>> David
>>>> 
>>>> Yumin Qi said the following on 12/11/09 08:50:
>>>>> Tom,
>>>>> 
>>>>> Thanks, changed as pointed.
>>>>> Sorry I forget to include GC group.
>>>>> 
>>>>> Can someone else give a look?
>>>>> 
>>>>> http://cr.openjdk.java.net/~minqi/6361589
>>>>> 
>>>>> 
>>>>> 
>>>>> Thanks
>>>>> Yumin
>>>>> 
>>>>> Tom Rodriguez wrote:
>>>>>> Overall this looks pretty nice.
>>>>>> 
>>>>>> java_thread_gc isn't a very clear name.  Maybe collected_thread or processed_thread?  The comment should describe it's purpose more explicitly.
>>>>>> 
>>>>>> Is this assert really needed?
>>>>>> 
>>>>>> +    assert(thread != this, "Should not be java thread itself");
>>>>>> 
>>>>>> I think you should add is_NamedThread() instead of distinguishing all these cases individually.
>>>>>> 
>>>>>> If VMThread is now a NamedTread why don't you call set_name instead of having it override name again?
>>>>>> 
>>>>>> vmError.cpp:
>>>>>> 
>>>>>> I think the printing output should look more like this:
>>>>>> 
>>>>>> +         st->print_cr("JavaThread " PTR_FORMAT " was being processed during the crash, id = " UINTX_FORMAT, jt, jt->osthread()->thread_id());
>>>>>> 
>>>>>> tom
>>>>>> 
>>>>>> On Dec 9, 2009, at 2:49 PM, Yumin Qi wrote:
>>>>>> 
>>>>>> 
>>>>>>> Fixed 6361589: Print out stack trace for target thread of GC crash
>>>>>>> 
>>>>>>> http://cr.openjdk.java.net/~minqi/6361589
>>>>>>> 
>>>>>>> Comments: This is a facility to assist for JVM crash analysis. To implement this function, it needs record JavaThread which may involve crash somewhere in GC thread. The place chosen here is NamedThread from which all GC threads but VMThread derive. I change VMThread's base from Thread to NamedThread, so the recording of JavaThread covers for GC threads and VMThread. Once the JavaThread recorded in GC thread seen during error report, its stack trace be printed out. To test this function, add a debug flag CrashGCForDumpingJavaThread, once it is set, it will crash immediately in VMThread or GC Thread to core dump. Note this is for test only purpose, no defects in involved JavaThread.
>>>>>>> 
>>>>>>> 
>>>>>>> Thanks
>>>>>>> Yumin
>>>>>>>    
>>>>>> 
>>>>>>  
>>>>> 
>>> 
>> 
> 



More information about the hotspot-runtime-dev mailing list