[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Pls review 7091418: FX priority class from Solaris should be available to JVM )
From:       paul.hohensee () oracle ! com (Paul Hohensee)
Date:       2012-01-26 20:22:31
Message-ID: 4F21B607.1060409 () oracle ! com
[Download RAW message or body]

Thanks for the quick review. :)

Inline...

On 1/26/12 12:34 PM, Daniel D. Daugherty wrote:
> On 1/25/12 4:08 PM, Paul Hohensee wrote:
>> New webrev at
>>
>> http://cr.openjdk.java.net/~phh/7091418.01/
>
> Thumbs up!
>
>
> Don't forget to use this bug ID when you commit:
>
>     7082553 2/3 Interpret Thread.setPriority(Thread.MAX_PRIORITY) to mean
>                 FX60 on Solaris 10 and 11

Yes, I'll remember. :)

>
> It would be nice to update the copyrights...

Will do.

>
>
> src/os/bsd/vm/os_bsd.cpp
>     No comments.
>
> src/os/linux/vm/os_linux.cpp
>     No comments.
>
> src/os/solaris/vm/osThread_solaris.hpp
>     typo line 36: 'create' -> 'created'

Will fix.

>
> src/os/solaris/vm/os_solaris.cpp
>     nit line 3798 - deleted a blank line - why?

Source code hygiene (general rule, be able to see as much as
possible in an editor window).  I tend to sneak at least one of
these into every fix I make.  Too many, and people rightly say
they can't see the real change, but I can usually get away with
a few.  There's another one at 3831.

>
>     lines 3998-4031 are pretty domain specific; I can't vouch for the
>         correctness one way or the other

I had Steve Sistare in Solaris scheduler land look at these.

>
>     lines 4000, 4012, 4022 seem a bit long

Will fix.

>
>     nit lines 4036-7 - deleted blanks lines - why?

See above.

>
> src/os/windows/vm/os_windows.cpp
>     No comments.
>
> src/share/vm/compiler/compileBroker.cpp
>     No comments.
>
> src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepThread.cpp 
>
>     line 78: The comment on line 75 in os.hpp says that the VMThread
>         uses NearMaxPriority so this comment (nor the original
>         comment) make sense.

The original comment didn't make sense with the original code, so I left it
(both the comment and the original default) alone. :)  I'll change it to 
note
that if CriticalPriority is used then there's a small risk of VMThread
starvation (because it does indeed run at NearMaxPriority).  On T4, there's
no risk, because (1) there's a lot of extra cores, so there's going to be
a core to run the VMThread on, and (2) Solaris doesn't take CriticalPriority
as a command, but rather as advisory, so the VMThread won't starve.

On other platforms, there might be an issue, but that's why this is
experimental.

>
> src/share/vm/runtime/globals.hpp
>     nit line 3491: "VM thread" -> "the VM thread"

Will fix.

>
> src/share/vm/runtime/os.hpp
>     Why am I reminded of the scene in "Spinal Tap" where they
>     are talking about the amp that goes up to 11 instead of 10?

Exactly.

>
>     No (real) comments.

Thanks again,

Paul


[prev in list] [next in list] [prev in thread] [next in thread]