RFR(M): 8155949: Support relaxed semantics in cmpxchg
David Holmes
david.holmes at oracle.com
Tue May 24 20:19:30 UTC 2016
On 25/05/2016 2:03 AM, Zhengyu Gu wrote:
>
> On 05/24/2016 11:38 AM, Zhengyu Gu wrote:
>>
>>
>> On 05/24/2016 09:06 AM, Doerr, Martin wrote:
>>> Hi David,
>>>
>>> unfortunately, Atomic::add(jlong) is used by mallocTracker.hpp (e.g.
>>> line 56). Removing it breaks the build.
>> It should be replaced with size_t version in mallocTracker.hpp.
>>
> I created https://bugs.openjdk.java.net/browse/JDK-8157709 for this.
Thanks Zhengyu.
David
> -Zhengyu
>
>> -Zhengyu
>>
>>
>>
>>>
>>> But I could change it as follows:
>>> inline jlong Atomic::add(jlong add_value, volatile jlong* dest) {
>>> #ifdef _LP64
>>> return (jlong) add_ptr((intptr_t) add_value, (volatile intptr_t*)
>>> dest);
>>> #else
>>> jlong old = load(dest);
>>> jlong new_value = old + add_value;
>>> while (old != cmpxchg(new_value, dest, old)) {
>>> old = load(dest);
>>> new_value = old + add_value;
>>> }
>>> return new_value;
>>> #endif
>>> }
>>>
>>> Best regards,
>>> Martin
>>>
>>>
>>> -----Original Message-----
>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>> Sent: Dienstag, 24. Mai 2016 14:27
>>> To: Doerr, Martin <martin.doerr at sap.com>; Andrew Haley
>>> (aph at redhat.com) <aph at redhat.com>
>>> Cc: Hiroshi H Horii <HORII at jp.ibm.com>; Tim Ellison
>>> <Tim_Ellison at uk.ibm.com>; ppc-aix-port-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
>>>
>>> Hi Martin,
>>>
>>> On 24/05/2016 8:21 PM, Doerr, Martin wrote:
>>>> Hi David,
>>>>
>>>> it was moved for the same reason as the jint version of cmpxchg: It
>>>> passes the memory order to the jint version.
>>>> It may look large in terms of C++ code, but there's not much
>>>> substantial content.
>>>> I can only see a loop which calls the jint version + a bunch of very
>>>> simple operations.
>>>> Why shouldn't we give compilers a chance to inline and possibly
>>>> optimize some of the simple operations and especially to eliminate
>>>> the order check?
>>> I think this forces the compiler to inline it, not just "gives it a
>>> chance". But I'll leave it to those more knowledgeable about the
>>> compiler side of this to comment.
>>>
>>> But if we're making these changes can you delete the Atomic::add(jlong)
>>> - it is unused and incorrect as discussed here:
>>>
>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-February/021620.html
>>>
>>>
>>> Thanks,
>>> David
>>>
>>>> Best regards,
>>>> Martin
>>>>
>>>> -----Original Message-----
>>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>>> Sent: Dienstag, 24. Mai 2016 12:04
>>>> To: Doerr, Martin <martin.doerr at sap.com>; Andrew Haley
>>>> (aph at redhat.com) <aph at redhat.com>
>>>> Cc: Hiroshi H Horii <HORII at jp.ibm.com>; Tim Ellison
>>>> <Tim_Ellison at uk.ibm.com>; ppc-aix-port-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
>>>>
>>>> On 24/05/2016 7:37 PM, Doerr, Martin wrote:
>>>>> Hi David and Andrew,
>>>>>
>>>>> sorry for missing this one. There were too many emails.
>>>>>
>>>>> After moving the jint version as well, there was not much left of
>>>>> atomic.cpp.
>>>>> I think it doesn't make any sense to keep a couple of trivial
>>>>> functions in the cpp file.
>>>>> Therefore, I have removed atomic.cpp and moved the remaining small
>>>>> functions into the inline file.
>>>> Sorry I don't understand why the jbyte cmpxchg_general was moved to the
>>>> .inline.hpp file - it seems far too big to be inlined.
>>>>
>>>> David
>>>>
>>>>> Webrev is here:
>>>>> http://cr.openjdk.java.net/~mdoerr/8155949_relaxed_cas/webrev.05/
>>>>>
>>>>> Best regards,
>>>>> Martin
>>>>>
>>>>>
>>>>> -----Original Message-----
>>>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>>>> Sent: Dienstag, 24. Mai 2016 05:50
>>>>> To: Doerr, Martin <martin.doerr at sap.com>
>>>>> Cc: Hiroshi H Horii <HORII at jp.ibm.com>; Tim Ellison
>>>>> <Tim_Ellison at uk.ibm.com>; ppc-aix-port-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
>>>>>
>>>>> Hi Martin,
>>>>>
>>>>> On 23/05/2016 7:29 PM, Doerr, Martin wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> here's the new webrev:
>>>>>> http://cr.openjdk.java.net/~mdoerr/8155949_relaxed_cas/webrev.04/
>>>>> There seems to be some confusion. You've moved the jbyte
>>>>> Atomic::cmpxchg_general from the .cpp file to the .inline/hpp file,
>>>>> but
>>>>> the comments from Andrew and Kim were about moving the unsigned
>>>>> Atomic::cmpxchg version. ??
>>>>>
>>>>> Aside: In the changeset contributor's have to be specified by "email
>>>>> address" or "name <email address>", OpenJDK user names are not
>>>>> accepted.
>>>>> I think Andrew should also be listed there for the Aarch64 component.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Btw.: The jbyte version of cmpxchg can be implemented on aarch
>>>>>> like on ppc where we emulate the byte access by a 4 byte access
>>>>>> (lwarx/stwcx). But that should better be done in a separate change.
>>>>>>
>>>>>> Thanks for your time and your support.
>>>>>>
>>>>>> Best regards,
>>>>>> Martin
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>>>>> Sent: Samstag, 21. Mai 2016 01:10
>>>>>> To: Doerr, Martin <martin.doerr at sap.com>
>>>>>> Cc: Hiroshi H Horii <HORII at jp.ibm.com>; Tim Ellison
>>>>>> <Tim_Ellison at uk.ibm.com>; ppc-aix-port-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
>>>>>>
>>>>>> Hi Martin,
>>>>>>
>>>>>> Are you in a position to make the change now suggested by both Kim
>>>>>> and
>>>>>> Andrew? Can you also include the Aarch64 code that Andrew provided:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~aph/8154736
>>>>>>
>>>>>> I'd like to get this finalized so it is ready to push as soon as the
>>>>>> process allows it to.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>> On 20/05/2016 8:03 AM, Kim Barrett wrote:
>>>>>>>> On May 18, 2016, at 6:12 AM, Doerr, Martin
>>>>>>>> <martin.doerr at sap.com> wrote:
>>>>>>>>
>>>>>>>> Hi Kim,
>>>>>>>>
>>>>>>>> thank you very much for the detailed review.
>>>>>>>>
>>>>>>>> I agree with your comments and I have made all your requested
>>>>>>>> changes here:
>>>>>>>> http://cr.openjdk.java.net/~goetz/wr16/8155949-relaxed_cas/webrev.03/
>>>>>>>>
>>>>>>>>
>>>>>>>> It's correct that the change changes the semantics of the
>>>>>>>> conservative cmpxchg. In case of failure, we also execute the
>>>>>>>> sync instruction, now.
>>>>>>>> Advantage is that the new implementation is maximum conservative
>>>>>>>> by default. I think this makes sense as long as the semantics of
>>>>>>>> the hotspot C++ cmpxchg are not clearly specified.
>>>>>>>>
>>>>>>>> For performance optimization, we should better use (or introduce
>>>>>>>> additional) enum values.
>>>>>>> ------------------------------------------------------------------------------
>>>>>>>
>>>>>>> There doesn't seem to have been any change for this earlier comment.
>>>>>>>
>>>>>>> src/share/vm/runtime/atomic.cpp
>>>>>>> 59 unsigned Atomic::cmpxchg(unsigned int exchange_value,
>>>>>>> 60 volatile unsigned int* dest,
>>>>>>> unsigned int compare_value,
>>>>>>> 61 cmpxchg_memory_order order) {
>>>>>>>
>>>>>>> I'm surprised this was ever out-of-line. But with this change it's
>>>>>>> quite bad to be out-of-line, as that's going to kill the constant
>>>>>>> propogation of the order value.
>>>>>>>
>>>>>>> ------------------------------------------------------------------------------
>>>>>>>
>>>>>>>
>>>>>>> Other than that, looks good.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>
>
More information about the ppc-aix-port-dev
mailing list