RFR (M): 8184334: Generalizing Atomic with templates

[Andrew Haley]

On 02/08/17 04:35, David Holmes wrote:
> Hi Kim,
> 
> Good planning on Erik's part to go on vacation just as I have returned ;-)
> 
> On 1/08/2017 4:18 AM, Kim Barrett wrote:
>>> On Jul 28, 2017, at 12:25 PM, Erik Osterlund <erik.osterlund at oracle.com> wrote:
>>>
>>> In that case, feel free to propose a revised solution while I am gone.
>>
>> Erik has asked me to try to make progress on this while he's on
>> vacation, rather than possibly letting it sit until he gets back.
> 
> Okay, while Erik is gone perhaps you can clarify a few things. As Andrew 
> and Roman have expressed, I too find this:
> 
> +   template <typename T, typename U, typename V>
> +   inline static U cmpxchg(T exchange_value, volatile U* dest, V 
> compare_value, cmpxchg_memory_order order);
> 
> totally unintuitive and unappealing. I do not understand the rationale 
> for this this at all. It does not make any sense to me to allow T, U and 
> V to be different types (even if constrained). It has been stated that 
> if we force them to all be the same there is some problem with literals 
> and the need for casts, but I don't understand what that problem would be.

A couple of examples would help:

diff -r d207c56d5b5a src/share/vm/runtime/os.cpp
--- a/src/share/vm/runtime/os.cpp	Tue Jul 25 15:35:09 2017 +0100
+++ b/src/share/vm/runtime/os.cpp	Wed Aug 02 09:24:30 2017 +0100
@@ -756,7 +756,7 @@
   while (true) {
     unsigned int seed = _rand_seed;
     int rand = random_helper(seed);
-    if (Atomic::cmpxchg(rand, &_rand_seed, seed) == seed) {
+    if (Atomic::cmpxchg((unsigned)rand, &_rand_seed, seed) == seed) {
       return rand;
     }
   }
diff -r d207c56d5b5a src/share/vm/runtime/thread.cpp
--- a/src/share/vm/runtime/thread.cpp	Tue Jul 25 15:35:09 2017 +0100
+++ b/src/share/vm/runtime/thread.cpp	Wed Aug 02 09:24:30 2017 +0100
@@ -4741,7 +4741,7 @@
 enum MuxBits { LOCKBIT = 1 };

 void Thread::muxAcquire(volatile intptr_t * Lock, const char * LockName) {
-  intptr_t w = Atomic::cmpxchg(LOCKBIT, Lock, 0);
+  intptr_t w = Atomic::cmpxchg((intptr_t)LOCKBIT, Lock, (intptr_t)0);
   if (w == 0) return;
   if ((w & LOCKBIT) == 0 && Atomic::cmpxchg (w|LOCKBIT, Lock, w) == w) {
     return;

There are eight such changes required in the HotSpot codebase.

I strongly believe that these changes make the code better.  They show
exactly where types do not match.  In the case of the random
generator, they show that there is a type mismatch which is IMO
probably wrong, and it wouldn't hurt to fix it.

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671