RFR (M) 8186903: Remove j-types from Atomic

Thu Dec 7 13:11:01 UTC 2017

On 12/6/17 11:33 PM, David Holmes wrote:
> On 7/12/2017 2:00 PM, coleen.phillimore at oracle.com wrote:
>> On 12/6/17 9:41 PM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> On 7/12/2017 7:59 AM, coleen.phillimore at oracle.com wrote:
>>>> Summary: Change parameter types from jlong, jint, jbyte to int64_t, 
>>>> int32_t and int8_t respectively
>>>
>>> That's fine. But you also made a bunch of changes to rename the 
>>> "ptr" variants to "long", and in the process changed intptr_t to 
>>> int64_t. The change ptr->long isn't really accurate. And intptr_t 
>>> and int64_t are different sizes on 32-bit! So this change seems 
>>> problematic and out of scope by your own description. That said I 
>>> thought we had/were getting rid of the "ptr" variants ??
>>
>> Yes, I did rename atomic_add_ptr_entry to atomic_add_long_entry. This 
>> is to match the other names which are currently empty, "long" and 
>> "byte". This was a last vestige of the ptr variants.   It's only used 
>> on 64 bit windows, so changing it to intptr_t to int64_t is 
>> correct.   For some reason, there isn't a long variant on 32 bits.  
>> Possibly because we don't add jlong values in the vm?
>
> Okay. Was hard to see this was all on 64-bit only.
>
> Though not clear why this renaming needs to happen at all at this 
> time? I'm finding it very confusing trying to see where we are going 
> with this - shouldn't all the ptr variants become unused with the new 
> Atomic API? I find them easier to spot when written as _ptr_ rather 
> than _long_. (And on Windows a long isn't 64-bits anyway!)

Yes, all the ptrs variants were removed in a previous change but I'd 
missed this renaming this one to long.  Since I was fixing the arguments 
for this function, I took the opportunity to also change the name in the 
same line.  I could file a new RFE for this and separate out the few 
lines.  The _long_ matches the other function names, and this case is 
actually used on _long_ because by this point the pointers have been 
normalized by template magic to long when they get here.   See the other 
related functions in this change:

http://cr.openjdk.java.net/~coleenp/8186903.01/webrev/src/hotspot/os_cpu/windows_x86/os_windows_x86.hpp.udiff.html
>
>> ent
>> So this renaming makes it consistent with the other names, which then 
>> can all be changed at once, when nice names are agreed upon.
>>>
>>>> Leave renaming functions for later change.
>>>>
>>>> Ran JPRT which builds more Oracle platforms, mach5 tier1-5 on 
>>>> windows/linux x64 and tier1 on solaris-sparcv9.  Also built Zero 
>>>> product mode (fails building fastdebug for some other reason).
>>>>
>>>> Other platforms: could you please test this patch?
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8186903.01/webrev
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8186903
>>>>
>>>> This change is for JDK 11.
>>>
>>> src/hotspot/cpu/x86/stubGenerator_x86_32.cpp
>>>
>>> Return type should be int32_t not int
>>>
>>> src/hotspot/os_cpu/windows_x86/os_windows_x86.hpp
>>>
>>> -  static intptr_t  (*atomic_xchg_long_func)     (jlong, volatile 
>>> jlong*);
>>> -  static intptr_t  atomic_xchg_long_bootstrap   (jlong, volatile 
>>> jlong*);
>>>
>>> Wow - that looks like a bug! Only returns 32-bits on 32-bit!
>>
>> This one also is only used on 64 bit.  I have to confess I don't 
>> follow all the paths of the atomics and their various translations 
>> down to the platform levels.   I'm not sure how windows 32 bit calls 
>> atomic_add or xchg for 64 bit values (or if it is not implemented).
>
> Not implemented at the lowest levels AFAICS - only load/store and 
> cmpxchg<8>. Of course the other atomics can then be written at a 
> higher-level in terms of cmpxchg.
>
> The whole use of the stubgenerator for atomics is something that needs 
> re-examining as I think it is completely unnecessary today (and likely 
> for quite some time). The opening comment in atomic_windows_x86.hpp is 
> somewhat startling as we approach 2018 :)

I asked about this in the review and Erik said they were needed for some 
reason, but it would be nice to have these cleaned up somehow.   Kim has 
some ideas for simplifying the implementation. My script to find all 
these names goes something like this:

foreach atomic (cmpxchg xchg add load store)
...
      foreach type (entry func bootstrap)
...
           foreach ("" "long" "byte")
...

>
> // The following alternative implementations are needed because
> // Windows 95 doesn't support (some of) the corresponding Windows NT
> // calls. Furthermore, these versions allow inlining in the caller.
> // (More precisely: The documentation for InterlockedExchange says
> // it is supported for Windows 95. However, when single-stepping
> // through the assembly code we cannot step into the routine and
> // when looking at the routine address we see only garbage code.
> // Better safe then sorry!). Was bug 7/31/98 (gri).

Yeah, good thing we can single step on Windows 95!

Thanks,
Coleen

>
> Cheers,
> David
>
>>>
>>> Everything else seems fine.
>>>
>>
>> Thanks for the eagle eye review.
>> Coleen
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> Coleen
>>