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

[David Holmes]

Hi Paul,

Warning: long winded response below

Summary:
- file RFE to fully support "system" priority for VM internal threads
- Fully implement UseCriticalJavaThreadPriority on non-Solaris

You'll have to read on to understand what I mean by both of these :)

On 24/01/2012 8:00 AM, Paul Hohensee wrote:
> 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.

Thanks for reminding me about this CR - I knew there had been further 
discussion on this. That CR is not public, which seems a little 
pointless now.

I'll restate my concerns as outlined in that CR. It is very difficult 
for an application programmer to determine that FX60 will provide any 
performance benefit, whether assigned to a compiler thread, GC thread or 
Java thread. The benefits of FX60 only exist when a system has idle 
cores, so the application programmer and/or the application "deployer" 
has to know what overall load will exist on the system. For the GC and 
compiler threads this is not such an issue as the person deploying the 
app can simply experiment with the flag being on and off and see it it 
helps in their situation.

For Java threads however the application programmer has to use 
Thread.MAX_PRIORITY. This has two negative consequences. First it 
encourages people to start thinking about Thread priorities when we have 
spent a dozen years telling them to forget about them because in 99% of 
the cases they have no meaning. For them to have meaning you have to 
select non default ThreadPriorityPolicy settings and use the right OS - 
even then you get different affects on different OSes. Which leads me to 
the second consequence: a loss of portability. Using Thread.MAX_PRIORITY 
can have different consequences on different platforms, even today.

Ok - so the response to the above is "These are experimental options 
which must be unlocked, and its for advanced users and 'caveat emptor'". 
I can live with that, so lets look at the details of the implementation.

There are really two distinct parts to this. First the ability to define 
a special system thread priority. In that regard I like the idea of 
creating a pseudo-priority of 11 and using that to map to this system, 
aka critical, priority (though this could be better documented). You 
then have flags the say "use the system priority for threads of type X". 
This is cross-platform. There are two things I find problematic with 
this though:

1. As priority is simply a number we can't directly use this to 
implement FX60. Instead we have to put in a hack where a negative value 
on Solaris tells the system specific code "that means FX 60".
2. There is no mechanism, apart from recompiling the VM, to actually 
control what this system/critical priority is.

So this general purpose mechanism falls somewhat short of being 
generally useful as a cross-platform mechanism. I find both of these 
unsatisfactory and a RFE should be filed to address them.

The second part is the "map Thread.MAX_PRIORITY to the critical 
priority" part. On Solaris you use UseCriticalJavaThreadPriority to 
actually update the priority mapping 10 -> critical. But you don't do 
this on the other platforms. This is what I object to - if you are going 
to make this a cross-platform setting then it should be implemented 
fully on all platforms. It doesn't matter that by default the end result 
is the same, the code should be there so that if you could define the 
critical priority at runtime things would work as expected.

Cheers,
David
-----

>
> 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
>>>>
>>