RFR (M) 8186903: Remove j-types from Atomic
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
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
>>> 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.
>>>> Return type should be int32_t not int
>>>> - static intptr_t (*atomic_xchg_long_func) (jlong, volatile
>>>> - static intptr_t atomic_xchg_long_bootstrap (jlong, volatile
>>>> 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!
>>>> Everything else seems fine.
>>> Thanks for the eagle eye review.
More information about the hotspot-runtime-dev