RFR (M) 8169061: Drop os::is_MP checks from Atomics

[Mikael Gerdin]

Hi Aleksey,

On 2017-04-24 14:53, Aleksey Shipilev wrote:
> Hi,
>
> See https://bugs.openjdk.java.net/browse/JDK-8169061.
>
> Our current Atomic code looks like this:
>
> // Adding a lock prefix to an instruction on MP machine
> #define LOCK_IF_MP(mp) "cmp $0, " #mp "; je 1f; lock; 1: "
>
> inline jint Atomic::add (jint add_value, volatile jint* dest) {
>   jint addend = add_value;
>   int mp = os::is_MP();
>   __asm__ volatile ( LOCK_IF_MP(%3) "xaddl %0,(%2)"
>                     : "=r" (addend)
>                     : "0" (addend), "r" (dest), "r" (mp)
>                     : "cc", "memory");
>   return addend + add_value;
> }
>
> Notice the LOCK_IF_MP part, which means we have flag reads, additional branch,
> etc. on the hot path. I would like us to consider dropping runtime os::is_MP
> checks from Atomics. The history of the original change predates OpenJDK, but I
> think it was deemed a plausible optimization in mostly-uniprocessor world.
> Today, this penalizes multi-core platforms without a good reason.
>
> Proposed change for jdk10/hs:
>   http://cr.openjdk.java.net/~shade/8169061/webrev.01/

I think this change makes a lot of sense. I'm not familiar enough with 
the details of the Atomic class to give you a thorough code review but I 
want to state that I support this change.

/Mikael

>
> It unconditionally does lock prefix on all atomics.
>
> Our interest for doing this is GC performance work. For example, in Shenandoah,
> native CASes are very frequently used for bitmap manipulation during marking,
> updating forwarding pointers for evacuating objects, updating references in the
> heap. We have measured around 1-2% GC time improvement with this change. I fully
> expect it to benefit G1 too, because it also does CASes on bitmaps.
>
> Testing: shenandoah tests, hotspot_runtime on Linux x86_64
>
> Putting this via the build-test system on other platforms would be appreciated!
>
> Thanks,
> -Aleksey
>