RFR: 8225788: Dead code in thread and safepoint

[David Holmes]

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.

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