RFR: 8225788: Dead code in thread and safepoint

[Robbin Ehn]

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/

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