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

Tue May 24 20:18:49 UTC 2016

On 24/05/2016 11:06 PM, Doerr, Martin wrote:
> Hi David,
>
> unfortunately, Atomic::add(jlong) is used by mallocTracker.hpp (e.g. line 56). Removing it breaks the build.

Yeah I only discovered that this morning when I checked my test build 
results. That in itself is a bug as Zhengyu has noted.

> But I could change it as follows:

No - thanks - lets just leave this part for another day.

Thanks,
David

> 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.
>>>>>
>>>>>
>>>>>
>>>>>