Pls review 7091418: FX priority class from Solaris should be available to JVM )

[Paul Hohensee]

I just found

7082553: Interpret Thread.setPriority(Thread.MAX_PRIORITY) to mean FX60 
on Solaris 10 and 11

which seems like the CR I should actually be using.

Ref your last comment on 7082553, we could indeed change the definition 
of JavaPriority10_To_OSPriority, but that would cover only Java threads, not
compiler or the CMS background threads.

Paul

On 1/23/12 2:11 PM, Paul Hohensee wrote:
> Thanks for the review.
>
> Inline...
>
> On 1/22/12 7:39 PM, David Holmes wrote:
>> Hi Paul,
>>
>> The meta-comment here is that there needs to be a clear description 
>> of what "critical priority" means and what constraints there are on 
>> setting it to some OS specific value. For example the current changes 
>> uses the FX scheduling class, but what if someone used the RT 
>> scheduling class instead? Would that work? Probably not, in which 
>> case we should document that this selection of the "critical 
>> priority" is not an arbitrary choice that can be made.
>>
>> Even for FX/60 I'm not certain that using this for Java threads might 
>> not prevent safepoints from being reached or induce some other form 
>> of livelock.
>
> I added material to the Comments field of the CR.
>
> I don't think there's a livelock problem with Java threads, because 
> Solaris takes
> FX60 as advisory, not as a command.  All that should happen is that a 
> critical
> priority Java thread will get to the safepoint earlier than 
> non-critical ones.
> I suppose it's possible for critical priority CMS or compiler threads 
> to starve
> non-critical Java threads, but they run at NearMaxPriority by default 
> now,
> which can do the same thing.  This is definitely an "expert-only" feature
> though, which is why it's experimental for the time being.
>>
>> On 21/01/2012 3:13 AM, Paul Hohensee wrote:
>>> Webrev here
>>>
>>> http://cr.openjdk.java.net/~phh/7091418.00/
>>>
>>> This change defines a new Java pseudo-priority called
>>> CriticalPriority, just above MaxPriority. Compiler threads, the CMS
>>> background thread, and Java threads can have the os equivalent of
>>> this priority. On Solaris, this is the FX/60 scheduling
>>> class/priority. On other platforms, it's the same as MaxPriority's os
>>> priority.
>>
>> For reference this is why the mapping to FX/60 has been proposed:
>>
>> http://blogs.oracle.com/observatory/entry/critical_threads_optimization
>>
>> I still don't fully grok what this optimization does in a general 
>> sense and it seems to be geared to providing better single-threaded 
>> performance on near-idle systems - which doesn't make any sense to me 
>> in a JVM context. But FX/60 also gives you true priority over TS/IA 
>> threads so that may be where the gain comes from. I wonder if any 
>> experiments were actually done using FX/59 rather than the "magical" 
>> FX/60?
>
> It's meant to be Solaris-Sparc-specific, but it was easier to 
> implement as a
> general feature than to specialize it.  Given enough cores, FX60 does 
> indeed
> give you true priority over TS/IA threads.  If there aren't enough cores
> to run both critical threads in single-thread mode and non-critical 
> threads
> at the same time, Solaris will allow non-critical threads to run on the
> same core(s) as critical ones.
>
> I don't know of any FX59 experiments, but given the amount of work 
> it's taken
> for the Solaris folks to get FX60 working, I doubt using it would have 
> any positive
> effect.
>>
>>> There are 3 new command line switches, all gated by
>>> UseExperimentalVMOptions.
>>>
>>> -XX:+UseCriticalJavaThreadPriority
>>>
>>> Maps Java MAX_PRIORITY to critical priority.
>>
>> I found what you have done here to be very confusing. The only place 
>> UseCriticalJavaThreadPriority is used is on Solaris. There you re-map 
>> the priority mapping for priority 10 to the "critical priority" as 
>> described.
>
> It's actually used on the other OSs.  It just maps to MaxPriority on 
> those.
>
>>
>> On all platforms you added an entry to the priority mapping table(s) 
>> for a non-existent Java priority 11. This provides a way to lookup 
>> the "critical priority" for the CMS/Compiler threads - in essence use 
>> of critical priority for those threads says "pretend these have Java 
>> priority 11" and then you've added a mapping for a priority 11 that 
>> is the same as for priority 10 except on Solaris. On Solaris you had 
>> to use a sentinel value to say "this really means use the "critical 
>> priority" because there is no way to convey a change of scheduling 
>> class.
>>
>> It seems to me that we are pretending to have "critical priority" 
>> support on all platforms when in reality we don't. If we want to go 
>> that way then we should extend it to the 
>> UseCriticalJavaThreadPriority case as well. It should be all or nothing.
>
> Extend it beyond making CriticalPriority == MaxPriority on non-Solaris 
> platforms?
> I.e., we can now change the compiler and CMS thread priority to 
> MaxPriority on
> non-Solaris platforms.  I don't know how to make CriticalPriority 
> higher than that
> on non-Solaris platforms.
>
>>
>> Further it needs to be made clear that these may still be dependent 
>> on the value of ThreadPriorityPolicy.
>
> I added a comment to the CR to that effect.
>
>>
>>> -XX:+UseCriticalCompilerThreadPriority
>>>
>>> All compiler threads run at critical priority.
>>
>> It should be more clear that UseCriticalCompilerThreadPriority only 
>> applies if CompilerThreadPriority is not set. Perhaps there should 
>> also be a startup check for both being used?
>
> I could, but making CompilerThreadPriority rule is what I intended.  I'll
> add a comment to globals.hpp and the CR.
>
>>
>> Thinking more though we really shouldn't need both flags. The basic 
>> problem is that the current "api" only supports setting a simple 
>> number and to use FX/60 also requires a change of scheduling class. 
>> You could add a hack that CompilerThreadPriority=60 means FX/60. Or, 
>> as I've suggested in past email we could generalize the format of the 
>> option to allow both a scheduling class designator and priority to be 
>> passed - that would be a more general mechanism.
>
> I didn't want to remove CompilerThreadPriority or change it's effect.  
> I can file a CR
> to do that though.  Current uses of CompilerThreadPriority=60 should 
> work like
> they always have.
>
> I wanted to confine the change as much as possible to Solaris _and_ to 
> limit it
> to just scheduling classes where we know we're not likely to provoke 
> thread
> starvation.  I can file a CR to add the ability to specify a 
> scheduling class for
> Java threads.  It would probably add 10 switches for scheduling class 
> corresponding
> to the existing 10 Java priority switches.  I don't have any ideas on 
> how to
> designate particular threads for particular class/priorities.
>
>>
>> Adding a psuedo-priority 11 is just means to work within the current 
>> limitations of the priority scheme.
>
> Correct.
>
>>
>>> -XX:+UseCriticalCMSThreadPriority
>>>
>>> The CMS background thread runs at critical priority.
>>
>> This doesn't make a lot of sense when you consider the comments in
>>
>> src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepThread.cpp 
>>
>>
>> which still states:
>>
>> "Priority should be just less than that of VMThread"
>>
>> This seems to indicate that we don't really understand what the 
>> priority relationship between GC threads and the VMThread should be.
>
> No, we don't.  That's why this is experimental.
>
>>
>> Should we be able to run the VMThread at FX/60?
>
> Perhaps.  It only matters for things like serial gc, which isn't used 
> on big iron.
>
>>
>>> On Solaris, one must in addition use -XX:+UseThreadPriorities to use 
>>> native
>>> priorities at all. Otherwise, Hotspot just accepts whatever Solaris
>>> decides.
>>
>> Is it also dependent on the value of ThreadPriorityPolicy? Should it 
>> be? Does it make sense to use it with either policy value?
>
> No, it's not dependent on ThreadPriorityPolicy.  Critical priority is 
> the same
> no matter what the default MaxPriority java_to_os_priority is.  I 
> think that's
> the right thing to do.
>
>>
>>>
>>> Before this change, the Solaris implementation could only change 
>>> priorities
>>> within the process scheduling class. It didn't change scheduling 
>>> classes on
>>> a per-thread basis. I added that capability and used it for the 
>>> critical
>>> thread
>>> work. I also fixed a bug where we were using thr_setprio() to save the
>>> original native priority during thread creation and reading it back 
>>> when
>>> the thread started via thr_getprio(). Since thr_setprio() can change 
>>> the
>>> user-supplied priority, this resulted in an unintended (lower) priority
>>> being used.
>>
>> I don't quite follow this. We used thr_setprio to set the native OS 
>> priority, and we then read it back using thr_getprio and then used 
>> that to pass to thr_setprio again (and also set_lwp_priority). If 
>> thr_setprio can change the user-supplied priority then it can make 
>> that change on the second call too can't it? Does the fact we now 
>> have a lwp affect this? I'm curious about the fact we still both use 
>> thr_setprio and set the LWP priority directly ???
>
> Possibly someone like Dave Dice can answer that question.  We were 
> already using
> both thr_setprio and set_lwp_priority together.  Likely that was in 
> case set_lwp_priority
> wasn't available.
>
> thr_setprio takes a value between 0 and 127 and map that to "some 
> priority" that
> may not be the same as its argument.  You can, for example, pass it 
> 127 and
> get 60 back from thr_getprio.  So if we set it once with 127 and then 
> set it again
> with 60, we can ultimately get back 0.  Which is what actually used to 
> happen.
>
> Paul
>>
>> Cheers,
>> David
>>
>>> Thanks,
>>>
>>> Paul
>>>
>