RFR: 8225788: Dead code in thread and safepoint

[Robbin Ehn]

Thanks Dan!

/Robbin

On 2019-08-14 19:49, Daniel D. Daugherty wrote:
> 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
>