RFR(M): 8155949: Support relaxed semantics in cmpxchg

David Holmes david.holmes at oracle.com
Thu May 12 09:52:14 UTC 2016 

On 12/05/2016 6:50 PM, Lindenmaier, Goetz wrote:
> Hi,
>
> atomic_bsd_zero.inline.hpp:303
> The order argument is not passed on to the inner cmpxchg_ptr call.
> But I guess this is not really relevant as the argument is not used

Right - that pattern is used throughout the changes.

> anyways.  (This method should be moved to the shared atomic.inline.hpp
> file, but not in this change.)
>
> Besides that the change looks good.   Reviewed.
>
> In case this now really is too late (http://openjdk.java.net/projects/jdk9/ states 26.5. for dev close, but hs is more early?)
> will there be jdk10 repos soon, or jdk9u?

Yes hs has to finalize sooner as the FC date is for things to be in 
jdk9/jdk9 and it takes time for changes to get from hs to jdk9.

There will be a process for requesting approval for changes post FC but 
that hasn't yet been announced either.

No word yet on when jdk10 forests will open up.

David
-----

> Best regards,
>   Goetz.
>
>
>
>
>
>> -----Original Message-----
>> From: hotspot-gc-dev [mailto:hotspot-gc-dev-bounces at openjdk.java.net]
>> On Behalf Of David Holmes
>> Sent: Donnerstag, 12. Mai 2016 00:50
>> To: Doerr, Martin <martin.doerr at sap.com>; Hiroshi H Horii
>> <HORII at jp.ibm.com>
>> Cc: Tim Ellison <Tim_Ellison at uk.ibm.com>; ppc-aix-port-
>> dev at openjdk.java.net; hotspot-dev developers <hotspot-
>> dev at openjdk.java.net>; hotspot-gc-dev at openjdk.java.net; hotspot-
>> runtime-dev at openjdk.java.net
>> Subject: Re: RFR(M): 8155949: Support relaxed semantics in cmpxchg
>>
>> This has about 3 hours to be reviewed and pushed to make the FC deadline.
>>
>> David
>>
>> On 11/05/2016 2:41 PM, David Holmes wrote:
>>> Adding hotspot-dev to cc to expand scope of reviewer pool :)
>>>
>>> On 11/05/2016 6:56 AM, David Holmes wrote:
>>>> On 11/05/2016 12:27 AM, Doerr, Martin wrote:
>>>>> Hello everybody,
>>>>>
>>>>> thanks for finding this issue. New webrev is here:
>>>>>
>>>>> http://cr.openjdk.java.net/~mdoerr/8155949_relaxed_cas/webrev.02/
>>>>
>>>> Unfortunately my test run hit a crash on Solaris sparc:
>>>>
>>>> # Problematic frame:
>>>> # V  [libjvm.so+0xcc35c4]
>>>> markOopDesc*markOopDesc::displaced_mark_helper()const+0x64
>>>>
>>>> I'm going to have to do some more testing to see if that is actually
>>>> related to the change. I know it should not be, but given we CAS marks I
>>>> have to wonder if there's some subtle bad interaction here. :(
>>>
>>> Further testing has not shown any failures on Solaris sparc, and the
>>> same testing showed some spurious failures on other platforms even
>>> without these changes. So while I will file a bug for this crash I think
>>> it unlikely to be related to the current changes.
>>>
>>> So on that note we just need a second hotspot reviewer to sign off on this.
>>>
>>> Thanks,
>>> David
>>>
>>>
>>>> David
>>>> -----
>>>>
>>>>>
>>>>>
>>>>> Best regards,
>>>>>
>>>>> Martin
>>>>>
>>>>>
>>>>>
>>>>> *From:*Hiroshi H Horii [mailto:HORII at jp.ibm.com]
>>>>> *Sent:* Dienstag, 10. Mai 2016 15:18
>>>>> *To:* David Holmes <david.holmes at oracle.com>
>>>>> *Cc:* hotspot-gc-dev at openjdk.java.net;
>>>>> hotspot-runtime-dev at openjdk.java.net; ppc-aix-port-
>> dev at openjdk.java.net;
>>>>> Tim Ellison <Tim_Ellison at uk.ibm.com>; Doerr, Martin
>>>>> <martin.doerr at sap.com>
>>>>> *Subject:* Re: RFR(M): 8155949: Support relaxed semantics in cmpxchg
>>>>>
>>>>>
>>>>>
>>>>> Hi David,
>>>>>
>>>>>> Just need another reviewer to chime in - given you and Martin are both
>>>>>> contributors. Or are you the main contributor with Martin being a
>>>>>> reviewer?
>>>>>
>>>>> Martin and I are contributors of this change.
>>>>>
>>>>>> Still a problem on Solaris sparc:
>>>>>
>>>>> Martin, could you create a new change in webrev with the patch that
>>>>> David sent?
>>>>>
>>>>> Regards,
>>>>> Hiroshi
>>>>> -----------------------
>>>>> Hiroshi Horii, Ph.D.
>>>>> IBM Research - Tokyo
>>>>>
>>>>>
>>>>> David Holmes <david.holmes at oracle.com
>> <mailto:david.holmes at oracle.com>>
>>>>> wrote on 05/10/2016 21:29:53:
>>>>>
>>>>>> From: David Holmes <david.holmes at oracle.com
>>>>>> <mailto:david.holmes at oracle.com>>
>>>>>> To: Hiroshi H Horii/Japan/IBM at IBMJP, "hotspot-runtime-
>>>>>> dev at openjdk.java.net <mailto:dev at openjdk.java.net>"
>>>>> <hotspot-runtime-dev at openjdk.java.net
>>>>> <mailto:hotspot-runtime-dev at openjdk.java.net>>
>>>>>> Cc: Tim Ellison <Tim_Ellison at uk.ibm.com
>>>>>> <mailto:Tim_Ellison at uk.ibm.com>>, "ppc-aix-port-
>>>>>> dev at openjdk.java.net <mailto:dev at openjdk.java.net>"
>>>>> <ppc-aix-port-dev at openjdk.java.net
>>>>> <mailto:ppc-aix-port-dev at openjdk.java.net>>, "hotspot-
>>>>>> gc-dev at openjdk.java.net <mailto:gc-dev at openjdk.java.net>"
>>>>> <hotspot-gc-dev at openjdk.java.net
>>>>> <mailto:hotspot-gc-dev at openjdk.java.net>>
>>>>>> Date: 05/10/2016 21:31
>>>>>> Subject: Re: RFR(M): 8155949: Support relaxed semantics in cmpxchg
>>>>>>
>>>>>> On 10/05/2016 9:04 PM, David Holmes wrote:
>>>>>>> Hi Hiroshi,
>>>>>>>
>>>>>>> On 10/05/2016 8:44 PM, Hiroshi H Horii wrote:
>>>>>>>> Hi All,
>>>>>>>>
>>>>>>>> Can I please request reviews for the following change?
>>>>>>>>
>>>>>>>> Code change:
>>>>>>>>
>> http://cr.openjdk.java.net/~mdoerr/8155949_relaxed_cas/webrev.01/
>>>>>>>
>>>>>>> Changes look good. I'm currently running them through our internal
>>>>>> build
>>>>>>> system. I will sponsor this and push the change through JPRT.
>>>>>>
>>>>>> Still a problem on Solaris sparc:
>>>>>>
>>>>>> "/opt/jprt/T/P1/102505.daholme/s/hotspot/src/share/vm/runtime/
>>>>>> atomic.inline.hpp",
>>>>>> line 96: Error: Could not find a match for static
>>>>>> Atomic::cmpxchg(signed
>>>>>> char, volatile signed char*, signed char).
>>>>>> 1 Error(s) detected.
>>>>>>
>>>>>> Needs this patch:
>>>>>>
>>>>>> diff -r 68853ef19be9 src/share/vm/runtime/atomic.inline.hpp
>>>>>> --- a/src/share/vm/runtime/atomic.inline.hpp
>>>>>> +++ b/src/share/vm/runtime/atomic.inline.hpp
>>>>>> @@ -92,7 +92,7 @@
>>>>>>
>>>>>>   #ifndef VM_HAS_SPECIALIZED_CMPXCHG_BYTE
>>>>>>   // See comment in atomic.cpp how to override.
>>>>>> -inline jbyte Atomic::cmpxchg(jbyte exchange_value, volatile jbyte
>>>>>> *dest, jbyte comparand)
>>>>>> +inline jbyte Atomic::cmpxchg(jbyte exchange_value, volatile jbyte
>>>>>> *dest, jbyte comparand, cmpxchg_memory_order order)
>>>>>>   {
>>>>>>     return cmpxchg_general(exchange_value, dest, comparand);
>>>>>>   }
>>>>>>
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>> Just need another reviewer to chime in - given you and Martin are
>>>>>> both
>>>>>>> contributors. Or are you the main contributor with Martin being a
>>>>>> reviewer?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>> PS. It's my night now so I'll be signing off and will pick this up in
>>>>>>> the morning.
>>>>>>>
>>>>>>>> This change follows the discussion started from these mails.
>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-
>>>>>> April/018960.html
>>>>>>>>
>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-
>>>>>> April/019148.html
>>>>>>>>
>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-
>>>>>> May/019320.html
>>>>>>>>
>>>>>>>>
>>>>>>>> Description:
>>>>>>>> This change provides relaxed compare-and-exchange by introducing
>>>>>>>> relaxed memory order. As described in
>> atomic_linux_ppc.inline.hpp,
>>>>>>>> the current implementation of cmpxchg is fence_cmpxchg_acquire.
>>>>>>>> This implementation is useful for general purposes because twice
>>>>>> calls of
>>>>>>>> sync before and after cmpxchg will provide strict consistency.
>>>>>>>> However, they sometimes cause overheads because sync
>> instructions
>>>>>> are
>>>>>>>> very expensive in the current POWER chip design.
>>>>>>>>
>>>>>>>> We confirmed this change improves performance of
>> copy_to_survivor
>>>>>>>> in the parallel GC. However, we will need more investigation of GC
>>>>>>>> by more experts. So, We would like to request a review of the
>> change
>>>>>>>> of cmpxchg first (as Martin requested).
>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-
>>>>>> April/019188.html
>>>>>>>>
>>>>>>>>
>>>>>>>> Summary of source code changes:
>>>>>>>>
>>>>>>>> * src/share/vm/runtime/atomic.hpp
>>>>>>>>      - Defines enum memory_order and adds a parameter to
>> cmpxchg.
>>>>>>>>
>>>>>>>> * src/share/vm/runtime/atomic.cpp
>>>>>>>> * src/os_cpu/bsd_x86/vm/atomic_bsd_x86.inline.hpp
>>>>>>>> * src/os_cpu/bsd_zero/vm/atomic_bsd_zero.inline.hpp
>>>>>>>> * src/os_cpu/linux_aarch64/vm/atomic_linux_aarch64.inline.hpp
>>>>>>>> * src/os_cpu/linux_sparc/vm/atomic_linux_sparc.inline.hpp
>>>>>>>> * src/os_cpu/linux_x86/vm/atomic_linux_x86.inline.hpp
>>>>>>>> * src/os_cpu/linux_zero/vm/atomic_linux_zero.inline.hpp
>>>>>>>> * src/os_cpu/solaris_sparc/vm/atomic_solaris_sparc.inline.hpp
>>>>>>>> * src/os_cpu/solaris_x86/vm/atomic_solaris_x86.inline.hpp
>>>>>>>> * src/os_cpu/windows_x86/vm/atomic_windows_x86.inline.hpp
>>>>>>>>      - Added a parameter for each cmpxchg function to follow
>>>>>>>>         the change of atomic.hpp. Their implementations are not
>>>>>> changed.
>>>>>>>>
>>>>>>>> * src/os_cpu/aix_ppc/vm/atomic_aix_ppc.inline.hpp
>>>>>>>> * src/os_cpu/linux_ppc/vm/atomic_linux_ppc.inline.hpp
>>>>>>>>      - Added a parameter for each cmpxchg function to follow
>>>>>>>>         the change of atomic.hpp. In addition, implementations
>>>>>>>>         are changed corresponding to the specified memory_order.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Hiroshi
>>>>>>>> -----------------------
>>>>>>>> Hiroshi Horii, Ph.D.
>>>>>>>> IBM Research - Tokyo
>>>>>>>>
>>>>>>
>>>>>

More information about the hotspot-runtime-dev mailing list