RFR (M): 8184334: Generalizing Atomic with templates

[David Holmes]

On 2/08/2017 6:31 PM, Andrew Haley wrote:
> 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.
> 

I agree. I don't think there is any problem having to coerce types to 
the correct form when they do not match directly. The casts make it 
explicit. Though I'm unsure how we manage to avoid the casts in the 
existing code in some of those cases.

Cheers,
David