review request for CR 6361589

[Coleen Phillimore]

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