RFR: 8225788: Dead code in thread and safepoint

[Daniel D. Daugherty]

On 8/9/19 9:16 AM, Robbin Ehn wrote:
> Hi David, Dan, Coleen,
>
> On 2019-06-21 07:52, David Holmes wrote:
>> Hi Robbin,
>>
>> Sorry for the extra long delay on getting back to this.
>
> Sorry for super extra long delay :)
>
> I addressed below and re-based it:
> (sorry no inc)
> http://cr.openjdk.java.net/~rehn/8225788/v2/webrev/

src/hotspot/share/compiler/compileBroker.cpp
     No comments.

src/hotspot/share/runtime/safepoint.cpp
     No comments.

src/hotspot/share/runtime/safepoint.hpp
     No comments.

src/hotspot/share/runtime/thread.cpp
     No comments.

src/hotspot/share/runtime/thread.hpp
     No comments.

src/hotspot/share/runtime/vframe_hp.cpp
     Don't forget to update the copyright year here.

src/hotspot/share/runtime/vmOperations.cpp
     No comments.

src/hotspot/share/runtime/vmOperations.hpp
     No comments.

src/hotspot/share/runtime/vmThread.cpp
     No comments.


I went back through the review thread and I think all the
mumbles I had in the earlier review have been resolved.

Thumbs up.

Dan



>
> I think the only loose end is the removal of:
> void trace_oops()                              PRODUCT_RETURN;
>
> Passes T1.
>
> Thanks, Robbin
>
>>
>> thread.cpp:
>>
>> You deleted JavaThread::java_priority(), but in os.cpp we have:
>>
>> // The mapping from OS priority back to Java priority may be inexact 
>> because
>> // Java priorities can map M:1 with native priorities. If you want 
>> the definite
>> // Java priority then use JavaThread::java_priority()
>>
>> So at a minimum the comment has to be updated. But pushing on 
>> ThreadPriority a little harder ... in thread.hpp we have:
>>
>>    // Prepare thread and add to priority queue.  If a priority is
>>    // not specified, use the priority of the thread object. Threads_lock
>>    // must be held while this function is called.
>>    void prepare(jobject jni_thread, ThreadPriority prio=NoPriority);
>>
>> The comment is out of date as no "priority queue" is involved 
>> anywhere. And the only call to JavaThread::prepare doesn't pass a 
>> ThreadPriority, so the parameter is unneeded and the logic in the 
>> implementation can just query the priority from the Thread object. 
>> That said, the code for that is:
>>
>> prio = java_lang_Thread::priority(thread_oop());
>>
>> and I would argue that that line should actually have been:
>>
>> prio = java_priority();
>>
>> thus making the function non-dead. As it is a single use I don't mind 
>> if the function goes but I think the assert it contained:
>>
>> assert(MinPriority <= priority && priority <= MaxPriority, "sanity 
>> check");
>>
>> should be added to the prepare() code.
>>
>> ---
>>
>> vmOperations.hpp
>>
>> The use of ThreadPriority on set_calling_thread without it being used 
>> seems very odd. I'll track down when that code got introduced. The 
>> "priority" used for the VM op queues are not thread priorities so 
>> unclear how this came about.
>>
>> Thanks,
>> David
>>
>> On 14/06/2019 6:08 am, David Holmes wrote:
>>> Hi Robbin,
>>>
>>> Sending out three RFRs after 6pm on my Friday night is not fair! I 
>>> will look at them all but not in detail till Monday :)
>>>
>>> This one was much larger than expected. Some surprises hiding in 
>>> there :) There are a couple of things where my initial reaction is 
>>> "hang on!" so I need to look at a couple of things more closely. :)
>>>
>>> E.g isn't JavaThread::trace_oops useful from the debugger?
>>>
>>> It also seems to me that there is further dead code based on some of 
>>> your changes E.g. I don't see _orig_thread_state actually being read 
>>> by anything.
>>>
>>> Cheers,
>>> David
>>>
>>>
>>> On 14/06/2019 10:29 pm, Robbin Ehn wrote:
>>>> Hi all, please review.
>>>>
>>>> Removing some dead code.
>>>>
>>>> Code:
>>>> http://cr.openjdk.java.net/~rehn/8225788/webrev/index.html
>>>> Issue:
>>>> https://bugs.openjdk.java.net/browse/JDK-8225788
>>>>
>>>> Thanks, Robbin