review request for CR 6361589

[Yumin Qi]

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