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

[David Holmes]

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.

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?

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

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.

Further it needs to be made clear that these may still be dependent on 
the value of ThreadPriorityPolicy.

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

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.

Adding a psuedo-priority 11 is just means to work within the current 
limitations of the priority scheme.

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

Should we be able to run the VMThread at FX/60?

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

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

Cheers,
David

> Thanks,
>
> Paul
>