Quantcast

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

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Aleksey Shipilev-4
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/

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Andrew Haley
On 24/04/17 13:53, Aleksey Shipilev wrote:
> 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.

I'm sure that you are right.  I (almost) completely ripped this out of
AArch64 too.

My reasoning was more to do with containers and the cloud.  Processors
can be dynamically added and removed, and this was enough to make me
shiver.  Having said that, there are still usages of is_MP in shared
code, and I see that I've missed removing it in one place in the
AArch64 sources.

Andrew.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mikael Gerdin
In reply to this post by Aleksey Shipilev-4
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
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
In reply to this post by Aleksey Shipilev-4
Hi Aleksey,

This looks good to me. I agree with assuming always MP for these atomic
ops. We should also look at what other os::is_MP() checks should be
deleted (different CR of course).

Small nit: atomic_solaris_x86.hpp - when you deleted text the
indentation of some declarations changed eg:

jbyte _Atomic_cmpxchg_byte(jbyte exchange_value, volatile jbyte* dest,
                      jbyte compare_value);

should be

jbyte _Atomic_cmpxchg_byte(jbyte exchange_value, volatile jbyte* dest,
                            jbyte compare_value);

Running it through JPRT.

Thanks,
David

On 24/04/2017 10:53 PM, 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/
>
> 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
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
One oversight in atomic_bsd_x86.hpp:

  118 inline void Atomic::inc_ptr(volatile intptr_t* dest) {
  119   __asm__ __volatile__ (  "lock addq $1,(%0)"
  120                         :
  121                         : "r" (dest), "r" (mp)

Forgot to delete "mp" reference.

Also copyright years need updating.

Thanks,
David

On 26/04/2017 11:17 AM, David Holmes wrote:

> Hi Aleksey,
>
> This looks good to me. I agree with assuming always MP for these atomic
> ops. We should also look at what other os::is_MP() checks should be
> deleted (different CR of course).
>
> Small nit: atomic_solaris_x86.hpp - when you deleted text the
> indentation of some declarations changed eg:
>
> jbyte _Atomic_cmpxchg_byte(jbyte exchange_value, volatile jbyte* dest,
>                      jbyte compare_value);
>
> should be
>
> jbyte _Atomic_cmpxchg_byte(jbyte exchange_value, volatile jbyte* dest,
>                            jbyte compare_value);
>
> Running it through JPRT.
>
> Thanks,
> David
>
> On 24/04/2017 10:53 PM, 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/
>>
>> 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
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Aleksey Shipilev-4
On 04/26/2017 05:29 AM, David Holmes wrote:

> One oversight in atomic_bsd_x86.hpp:
>
>  118 inline void Atomic::inc_ptr(volatile intptr_t* dest) {
>  119   __asm__ __volatile__ (  "lock addq $1,(%0)"
>  120                         :
>  121                         : "r" (dest), "r" (mp)
>
> Forgot to delete "mp" reference.
>
> Also copyright years need updating.

Right, thanks for running it!

I think I captured everything in here:
  http://cr.openjdk.java.net/~shade/8169061/webrev.02/

Thanks,
-Aleksey

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
Looks fine to me!

Thanks,
David
-----

On 26/04/2017 4:36 PM, Aleksey Shipilev wrote:

> On 04/26/2017 05:29 AM, David Holmes wrote:
>> One oversight in atomic_bsd_x86.hpp:
>>
>>  118 inline void Atomic::inc_ptr(volatile intptr_t* dest) {
>>  119   __asm__ __volatile__ (  "lock addq $1,(%0)"
>>  120                         :
>>  121                         : "r" (dest), "r" (mp)
>>
>> Forgot to delete "mp" reference.
>>
>> Also copyright years need updating.
>
> Right, thanks for running it!
>
> I think I captured everything in here:
>   http://cr.openjdk.java.net/~shade/8169061/webrev.02/
>
> Thanks,
> -Aleksey
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Aleksey Shipilev-4
Thanks! Would you mind sponsoring it?

Changeset:
  http://cr.openjdk.java.net/~shade/8169061/hotspot.changeset

-Aleksey

On 04/26/2017 08:42 AM, David Holmes wrote:

> Looks fine to me!
>
> Thanks,
> David
> -----
>
> On 26/04/2017 4:36 PM, Aleksey Shipilev wrote:
>> On 04/26/2017 05:29 AM, David Holmes wrote:
>>> One oversight in atomic_bsd_x86.hpp:
>>>
>>>  118 inline void Atomic::inc_ptr(volatile intptr_t* dest) {
>>>  119   __asm__ __volatile__ (  "lock addq $1,(%0)"
>>>  120                         :
>>>  121                         : "r" (dest), "r" (mp)
>>>
>>> Forgot to delete "mp" reference.
>>>
>>> Also copyright years need updating.
>>
>> Right, thanks for running it!
>>
>> I think I captured everything in here:
>>   http://cr.openjdk.java.net/~shade/8169061/webrev.02/
>>
>> Thanks,
>> -Aleksey
>>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
On 26/04/2017 5:08 PM, Aleksey Shipilev wrote:
> Thanks! Would you mind sponsoring it?

No problem.

David

> Changeset:
>   http://cr.openjdk.java.net/~shade/8169061/hotspot.changeset
>
> -Aleksey
>
> On 04/26/2017 08:42 AM, David Holmes wrote:
>> Looks fine to me!
>>
>> Thanks,
>> David
>> -----
>>
>> On 26/04/2017 4:36 PM, Aleksey Shipilev wrote:
>>> On 04/26/2017 05:29 AM, David Holmes wrote:
>>>> One oversight in atomic_bsd_x86.hpp:
>>>>
>>>>  118 inline void Atomic::inc_ptr(volatile intptr_t* dest) {
>>>>  119   __asm__ __volatile__ (  "lock addq $1,(%0)"
>>>>  120                         :
>>>>  121                         : "r" (dest), "r" (mp)
>>>>
>>>> Forgot to delete "mp" reference.
>>>>
>>>> Also copyright years need updating.
>>>
>>> Right, thanks for running it!
>>>
>>> I think I captured everything in here:
>>>   http://cr.openjdk.java.net/~shade/8169061/webrev.02/
>>>
>>> Thanks,
>>> -Aleksey
>>>
>
>
Loading...