review request for CR 6361589

[David Holmes - Sun Microsystems]

Looks good to me Yumin.

Thanks,
David

Yumin Qi said the following on 12/11/09 14:10:
> 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
>>>>>>>     
>>>>>>
>>>>>>   
>>>>>
>>>
>>
>