[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8152905: hs_err file is missing gc threads
From:       "Daniel D. Daugherty" <daniel.daugherty () oracle ! com>
Date:       2016-03-30 20:30:01
Message-ID: 56FC3749.9010900 () oracle ! com
[Download RAW message or body]

On 3/30/16 2:20 PM, Derek White wrote:
> Hi Dan,
>
> On 3/30/16 11:44 AM, Daniel D. Daugherty wrote:
>> On 3/29/16 9:32 PM, Derek White wrote:
>>> Summary: List the GC threads in the hs_err file in the "Other 
>>> Threads" section
>>>
>>> There are 4 small parts to this:
>>> 1) Fix G1CollectedHeap::gc_threads_do() to also iterate over 
>>> concurrent mark worker threads.
>>> 2) Have Thread::print_on_error() print the thread name of NamedThreads.
>>> 3) Factor out code that prints out each "Other Thread" to 
>>> Threads::print_on_error().
>>> 4) Call Threads::print_on_error() on every GC thread.
>>>
>>> BUG: 8152905: hs_err file is missing gc threads 
>>> <https://bugs.openjdk.java.net/browse/JDK-8152905>
>>> WEBREV: http://cr.openjdk.java.net/~drwhite/8152905/webrev.01/
>>
>> src/share/vm/runtime/thread.hpp
>>     No comments.
>>
>> src/share/vm/runtime/thread.cpp
>>     L831:   else st->print("Thread");
>>     L832:
>>     L833:   if (is_Named_thread()) {
>>     L834:     st->print(" \"%s\"", name());
>>     L835:   }
>>
>>         The new is_Named_thread() check is currently made for
>>         every thread type, but only applies to the else-statement
>>         on L831. That else-statement should change into an
>>         else-block. Perhaps:
>>
>>         else {
>>           st->print("Thread");
>>           if (is_Named_thread()) {
>>             st->print(" \"%s\"", name());
>>           }
>>         }
> The intention really is to print the name for every thread type. For 
> the GC threads, they often have the same type, but different purposes 
> reflected in the name. For example:
>   0x00007f77b807e800 ConcurrentGCThread "G1 Main Marker" [stack: 
> 0x00007f779ca7d000,0x00007f779cb7e000] [id=25865]
>   0x00007f77b8081000 ConcurrentGCThread "G1 Marker#0" [stack: 
> 0x00007f779c97c000,0x00007f779ca7d000] [id=25866]
>   0x00007f77b806a000 ConcurrentGCThread "G1 Refine#0" [stack: 
> 0x00007f779cee5000,0x00007f779cfe6000] [id=25863]
>   0x00007f77b806c000 ConcurrentGCThread "G1 Young RemSet Sampling" 
> [stack: 0x00007f779cde4000,0x00007f779cee5000] [id=25864]
>   0x00007f77b8296000 ConcurrentGCThread "G1 StrDedup" [stack: 
> 0x00007f770c757000,0x00007f770c858000] [id=25871]
>
> I can add explicit braces to the if-else-tree to make it clear that 
> this is not an oversight.

No need for the explicit braces. I simply misunderstood what you were
trying to do here.

Dan


>
> On a related note, I realized after I sent this that it's not safe to 
> call thread->name() on a JavaThread or CompilerThread in 
> Thread::print_on_error(), because the overridden name() might 
> allocate, but print_on_error() is also overridden, so it's safe after 
> all. I'll add asserts in Thread::print_on_error() that it's not called 
> on a JavaThread or CompilerThread.
>>
>>     L4496:   if (this_thread) {
>>         Should not use an implied boolean. This should be:
>>
>>         if (this_thread != NULL) {
> OK.
>>
>>     L4533:     bool is_current = (current == thread);
>>     L4534:     found_current = found_current || is_current;
>>     L4535:
>>     L4536:     st->print("%s", is_current ? "=>" : "  ");
>>     L4537:
>>     L4538:     st->print(PTR_FORMAT, p2i(thread));
>>     L4539:     st->print(" ");
>>     L4540:     thread->print_on_error(st, buf, buflen);
>>     L4541:     st->cr();
>>
>>         I think this block can refactor into:
>>
>>           print_on_error(thread, st, current, buf, buflen, 
>> &found_current);
>
> Oh, that works!
>>
>> src/share/vm/gc/g1/g1CollectedHeap.cpp
>>     No comments.
>>
>> src/share/vm/gc/g1/g1ConcurrentMark.cpp
>>     No comments.
>>
>> src/share/vm/gc/g1/g1ConcurrentMark.hpp
>>     No comments.
>>
>> Dan
>
> Thanks for the review! I'll post an updated webrev once tested.
>
>  - Derek
>>
>>
>>> TESTING:
>>>  - Tested "java -XX:ErrorHandlerTest=1 -version" on all collectors.
>>>  - jprt
>>>
>>> Thanks,
>>>
>>>  - Derek
>>>
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]