review request for CR 6361589

[Yumin Qi]

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