RE: jstack reports wrong thread priorities
Dmytro Sheyko
dmytro_sheyko at hotmail.com
Tue Sep 4 01:26:59 PDT 2012
Hi,
I would like to propose a test.
Regards,
Dmytro
> From: dmytro_sheyko at hotmail.com
> To: david.holmes at oracle.com
> Subject: RE: jstack reports wrong thread priorities
> Date: Tue, 4 Sep 2012 00:18:37 +0300
> CC: hotspot-dev at openjdk.java.net
>
>
> 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
> > >>>>
> > >>>>
> > >>
> > >
>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: T7194254.java
Url: http://mail.openjdk.java.net/pipermail/hotspot-dev/attachments/20120904/425943a4/T7194254-0001.java
More information about the hotspot-dev
mailing list