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

Fri Dec 8 12:38:45 UTC 2017

Thank you, David.

On 12/7/17 10:37 PM, David Holmes wrote:
> On 8/12/2017 1:28 AM, coleen.phillimore at oracle.com wrote:
>> 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 for clarifying that. :)
>
> No further comments from me. I hope all these "long" variants can 
> disappear in the near future. Along with stub generator use in Atomic :)

Me too, but I think some of this old code is in platforms Oracle doesn't 
support so may not have priority for clean up vs. removal.

thanks,
Coleen

>
> Thanks,
> David
>
>> 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
>>>>>
>>>
>>