RFR (S) 8217378: UseCriticalCMSThreadPriority is broken

[Daniel D. Daugherty]

On 1/18/19 6:53 AM, Aleksey Shipilev wrote:
> Bug:
>    https://bugs.openjdk.java.net/browse/JDK-8217378
>
> Fix:
>    http://cr.openjdk.java.net/~shade/8217378/webrev.01/

src/hotspot/share/runtime/os.cpp
     L220:   if ((p >= MinPriority && p <= MaxPriority) ||
     L221:        (p == CriticalPriority && 
thread->is_ConcurrentGC_thread())) {
         nit - please delete one indent space from L221.

         Copyright year needs to be updated.

test/hotspot/jtreg/gc/cms/TestCriticalPriority.java
     No comments.

Thumbs up. No need to see another webrev if you fix the nit on L221.

Dan


>
> While this concerns the experimental CMS option, the fix itself also unblocks Shenandoah RFE
> (JDK-8217343). There might be a larger discussion if CriticalPriority thingie is useful, but I would
> first push the simple fix that makes it work, at least to have the up-to-date performance data. It
> should make the most benefit on Solaris where critical prio can map to FX priorities, but
> non-Solaris builds can also enjoy the (control) GC threads priority elevated to MaxPriority, above
> the VMThread.
>
> Testing: new test, hotspot tier1, jdk-submit (running)
>
> Thanks,
> -Aleksey
>