RFR: 8225788: Dead code in thread and safepoint

[David Holmes]

Just to follow up ...

On 20/06/2019 10:52 pm, David Holmes wrote:
> Hi Robbin,
> 
> Sorry for the extra long delay on getting back to this.
> 
> 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.

So before Java 5 we used to try to do sophisticated "priority 
inheritance" involving the VMThread and safepoints. If my skimming of 
the code is right we would both boost thread priority to enable them to 
reach the safepoint sooner, and adapt the VMThread's priority based on 
the priority of the thread requesting the VM_operation - which is where 
set_calling_thread comes in. But in Java 5 that was all replaced with a 
much simpler scheme whereby the VMThread always runs at its own (higher) 
priority.

That old priority scheme also involved a number of calls to 
JavaThread::java_priority(). :)

Cheers,
David

> 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