RFR (M): 8184334: Generalizing Atomic with templates

[coleen.phillimore at oracle.com]

Hi,

I agree with the plea for simplicity, that the types in the templates 
should be the same as well, and code in hotspot should be cleaned up.  
The cast in os::random wouldn't have been necessary if the compiler had 
given me an error when I changed the types to int. I would have made 
that unsigned, maybe...

I find the IntegerTypes file reads like line noise (def: random 
characters you used to get from a misbehaving modem on your phone 
line).  I would likely avoid using any features in it because they'd 
cost me time to figure out which things to use.  The mental time cost of 
generalization is too high and I don't think we need to support 
something like atomic float operations (unless we do already?)

I do agree with moving away from the java types to native types though.  
This is worth the disruption (which doesn't seem like that much from the 
first patch).

Thanks,
Coleen

On 8/2/17 4:31 AM, 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.
>