RE: jstack reports wrong thread priorities
Dmytro Sheyko
dmytro_sheyko at hotmail.com
Mon Sep 3 14:18:37 PDT 2012
Hi David,
As for unbalanced API, you are right. It is not generally expected that set method comes alone without corresponding get method.
On the other hand I would prefer if getter return exactly the same value that was set by corresponding setter before. However, conversion from java priority to native is lossy. And os::get_priority may return not the same java priority that was passed to os::set_priority. Maybe in order to get rid of feeling that os::get_priority is missing we could just break get-set pattern and rename os::set_priority to something like os::use_java_priority or os::assign_java_priority.
I also think that when thread priority will be taken into account in VM_Operation, this will be likely original java priority (i.e. JavaThread::java_priority() for java threads and some constant NearMaxPriority/MaxPriority/CriticalPriority for non-java threads), but not deduced from native priority. Otherwise such prioritization will not work properly if OS does not support (or permit) distinct native priorities.
Anyway I am quite happy with the first patch since it solves the original issue.
Thank you,
Dmytro
> Date: Mon, 3 Sep 2012 15:44:58 +1000
> From: david.holmes at oracle.com
> To: dmytro_sheyko at hotmail.com
> CC: hotspot-dev at openjdk.java.net
> Subject: Re: jstack reports wrong thread priorities
>
> Hi Dmytro,
>
> I've moved 7194254 to the runtime area rather than serviceability as
> this is a VM issue not a jstack issue.
>
> The use of priority in VM_Operation was to allow prioritization of VM
> operation requests, but this is an unused facility.
>
> There is obviously an issue here, but there are some aspects of this
> that feel awkward to me. A balanced API has a getX to match a setX so it
> seems wrong to simply get rid of some of these methods rather than
> changing/fixing their semantics. os::set_priority is a wrapper around
> os::set_native_priority and has to map from a Java priority to a native
> one. Similarly os::get_priority is a wrapper around
> os::get_native_priority, and it has to convert from a native priority to
> a Java priority. It just turns out that the mapping is buggy in one case
> (where lesser numeric priority means higher priority) and imprecise when
> the mapping was a relation rather than a function. I don't think that in
> itself is a reason to kill the function - you just need to be careful
> where/how you use it.
>
> I'm more inclined to go with your original patch, of fixing the bug in
> os::get_priority and changing print_on to report the native priority and
> java_priority (which seemed to get lost in the second patch).
>
> Webrev of that original patch here:
>
> http://cr.openjdk.java.net/~dholmes/7194254/webrev.v1/
>
> Though the format specifier %lld needs changing - %lx seems to be used
> most commonly elsewhere for thread_id.
>
> Thanks,
> David
>
> On 1/09/2012 3:46 AM, Dmytro Sheyko wrote:
> >
> > Put new version of patch here: https://bugs.openjdk.java.net/show_bug.cgi?id=100275
> >
> >> From: dmytro_sheyko at hotmail.com
> >> To: david.holmes at oracle.com; hotspot-dev at openjdk.java.net
> >> Subject: RE: jstack reports wrong thread priorities
> >> Date: Fri, 31 Aug 2012 19:50:40 +0300
> >>
> >>
> >> Hi,
> >>
> >> I tried to figure out where os::get_priority is also used. I can see that besides Thread::print_on it is used in VMThread::execute,
> >> where thread priority is stored in VM_Operation using set_calling_thread. However stored priority is used nowhere. Therefore I believe that
> >> os::get_priority (with Thread::get_priority) can be removed. For JavaThread we can use java_priority(), for non java thread we should
> >> better use native priority rather than rough estimate of its java priority.
> >>
> >> Thanks,
> >> Dmytro
> >>
> >>> Date: Tue, 28 Aug 2012 09:56:35 +1000
> >>> From: david.holmes at oracle.com
> >>> To: dmytro_sheyko at hotmail.com
> >>> CC: hotspot-dev at openjdk.java.net
> >>> Subject: Re: jstack reports wrong thread priorities
> >>>
> >>> Hi Dmytro,
> >>>
> >>> Native priorities are a bit of a mess as you can tell. I like your
> >>> suggestion of reporting native priority plus Java priority distinctly,
> >>> and without this complex mapping scheme. But I think part of the problem
> >>> is that for some threads the priority is only manipulated at the native
> >>> level and so the Java priority never gets updated.
> >>>
> >>> The patch for os.cpp get_priority seems reasonable.
> >>>
> >>> David
> >>>
> >>> On 28/08/2012 1:46 AM, Dmytro Sheyko wrote:
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> Hi,
> >>>>
> >>>> I noticed that jstack on Linux for all threads always shows "prio=10" (which is wrong especially for threads that have NORM_PRIORITY, i.e. 5).
> >>>> It seems that os::get_priority() mistakenly assumes that for native priorities greater value corresponds to higher priority.
> >>>> This is not true for niceness, which is used as native priority for linux threads (where less value corresponds to higher priority).
> >>>> Thus os::get_priority() incorrectly translates native to java priority.
> >>>>
> >>>> Following patch fixes the problem (on Linux):
> >>>>
> >>>> diff -r de2aa86e037d src/share/vm/runtime/os.cpp
> >>>> --- a/src/share/vm/runtime/os.cpp Thu Aug 23 12:27:33 2012 -0700
> >>>> +++ b/src/share/vm/runtime/os.cpp Mon Aug 27 17:52:09 2012 +0300
> >>>> @@ -208,7 +208,12 @@
> >>>> OSReturn ret = get_native_priority(thread,&os_prio);
> >>>> if (ret != OS_OK) return ret;
> >>>>
> >>>> - for (p = MaxPriority; p> MinPriority&& java_to_os_priority[p]> os_prio; p--) ;
> >>>> + if (java_to_os_priority[MaxPriority]> java_to_os_priority[MinPriority]) {
> >>>> + for (p = MaxPriority; p> MinPriority&& java_to_os_priority[p]> os_prio; p--) ;
> >>>> + } else {
> >>>> + // niceness values are in reverse order
> >>>> + for (p = MaxPriority; p> MinPriority&& java_to_os_priority[p]< os_prio; p--) ;
> >>>> + }
> >>>> priority = (ThreadPriority)p;
> >>>> return OS_OK;
> >>>> }
> >>>>
> >>>>
> >>>> However jstack is still inaccurate about priorities on Windows. It shows "prio=6" for threads that have NORM_PRIORITY.
> >>>> The cause of such inaccuracy is that several java priorities are mapped to the same native priority. (E.g. 5 and 6 are mapped to THREAD_PRIORITY_NORMAL)
> >>>> Therefore I believe that instead of trying to figure out java priority by native priority we should better:
> >>>> 1. report native priority "as is" for all threads
> >>>> 2. report java priority only for java threads using "priority" field.
> >>>>
> >>>> Propose following patch:
> >>>>
> >>>> diff -r de2aa86e037d src/share/vm/runtime/thread.cpp
> >>>> --- a/src/share/vm/runtime/thread.cpp Thu Aug 23 12:27:33 2012 -0700
> >>>> +++ b/src/share/vm/runtime/thread.cpp Mon Aug 27 17:52:09 2012 +0300
> >>>> @@ -820,7 +820,11 @@
> >>>> void Thread::print_on(outputStream* st) const {
> >>>> // get_priority assumes osthread initialized
> >>>> if (osthread() != NULL) {
> >>>> - st->print("prio=%d tid=" INTPTR_FORMAT " ", get_priority(this), this);
> >>>> + int os_prio;
> >>>> + if (os::get_native_priority(this,&os_prio) == OS_OK) {
> >>>> + st->print("os_prio=%d ", os_prio);
> >>>> + }
> >>>> + st->print("tid=" INTPTR_FORMAT " ", this);
> >>>> osthread()->print_on(st);
> >>>> }
> >>>> debug_only(if (WizardMode) print_owned_locks_on(st);)
> >>>> @@ -2710,7 +2714,11 @@
> >>>> void JavaThread::print_on(outputStream *st) const {
> >>>> st->print("\"%s\" ", get_thread_name());
> >>>> oop thread_oop = threadObj();
> >>>> - if (thread_oop != NULL&& java_lang_Thread::is_daemon(thread_oop)) st->print("daemon ");
> >>>> + if (thread_oop != NULL) {
> >>>> + st->print("#%lld ", java_lang_Thread::thread_id(thread_oop));
> >>>> + if (java_lang_Thread::is_daemon(thread_oop)) st->print("daemon ");
> >>>> + st->print("prio=%d ", java_lang_Thread::priority(thread_oop));
> >>>> + }
> >>>> Thread::print_on(st);
> >>>> // print guess for valid stack memory region (assume 4K pages); helps lock debugging
> >>>> st->print_cr("[" INTPTR_FORMAT "]", (intptr_t)last_Java_sp()& ~right_n_bits(12));
> >>>>
> >>>> PS
> >>>> just filled bug report for the issue:
> >>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7194254
> >>>>
> >>>> Thanks,
> >>>> Dmytro
> >>>>
> >>>>
> >>
> >
More information about the hotspot-dev
mailing list