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

Thu Dec 7 15:28:33 UTC 2017

On 12/7/17 8:11 AM, coleen.phillimore at oracle.com wrote:
>
>
> 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:

Sorry, I meant _long_ matches other function names and this case is 
actually used on int64_t because the pointers have been normalized to 8 
byte int64_t, not _long_ because _long_ is 32 bits on windows.   So much 
confuse.

thanks,
Coleen

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