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

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
Summary: Change parameter types from jlong, jint, jbyte to int64_t,
int32_t and int8_t respectively

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.

Thanks,
Coleen
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Hi Coleen,

On 7/12/2017 7:59 AM, [hidden email] 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 ??

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

Everything else seems fine.

Thanks,
David

> Thanks,
> Coleen
Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore


On 12/6/17 9:41 PM, David Holmes wrote:

> Hi Coleen,
>
> On 7/12/2017 7:59 AM, [hidden email] 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?
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).
>
> Everything else seems fine.
>

Thanks for the eagle eye review.
Coleen
> Thanks,
> David
>
>> Thanks,
>> Coleen

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
On 7/12/2017 2:00 PM, [hidden email] wrote:

> On 12/6/17 9:41 PM, David Holmes wrote:
>> Hi Coleen,
>>
>> On 7/12/2017 7:59 AM, [hidden email] 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!)

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

// 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).

Cheers,
David

>>
>> Everything else seems fine.
>>
>
> Thanks for the eagle eye review.
> Coleen
>> Thanks,
>> David
>>
>>> Thanks,
>>> Coleen
>
Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore


On 12/6/17 11:33 PM, David Holmes wrote:

> On 7/12/2017 2:00 PM, [hidden email] wrote:
>> On 12/6/17 9:41 PM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> On 7/12/2017 7:59 AM, [hidden email] 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
>>

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore


On 12/7/17 8:11 AM, [hidden email] wrote:

>
>
> On 12/6/17 11:33 PM, David Holmes wrote:
>> On 7/12/2017 2:00 PM, [hidden email] wrote:
>>> On 12/6/17 9:41 PM, David Holmes wrote:
>>>> Hi Coleen,
>>>>
>>>> On 7/12/2017 7:59 AM, [hidden email] 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
>>>
>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
On 8/12/2017 1:28 AM, [hidden email] wrote:

> On 12/7/17 8:11 AM, [hidden email] wrote:
>>
>>
>> On 12/6/17 11:33 PM, David Holmes wrote:
>>> On 7/12/2017 2:00 PM, [hidden email] wrote:
>>>> On 12/6/17 9:41 PM, David Holmes wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> On 7/12/2017 7:59 AM, [hidden email] 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 :)

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
>>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

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

Andrew Dinn
In reply to this post by coleen.phillimore
On 06/12/17 21:59, [hidden email] wrote:
> Other platforms: could you please test this patch?
This built fine on AArch64 and successfully passed basic smoke tests
(runs javac and netbeans, passes jtreg tests for j.u.c).

I am currently running jcstress tests just to be sure. If you want to
wait on the result of that I will let you know the result when they
finish (all is ok so far but there is still a long way to go).

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore


On 12/8/17 6:52 AM, Andrew Dinn wrote:
> On 06/12/17 21:59, [hidden email] wrote:
>> Other platforms: could you please test this patch?
> This built fine on AArch64 and successfully passed basic smoke tests
> (runs javac and netbeans, passes jtreg tests for j.u.c).
>
> I am currently running jcstress tests just to be sure. If you want to
> wait on the result of that I will let you know the result when they
> finish (all is ok so far but there is still a long way to go).

Hi Andrew,  Did you also review the change?

There's no rush because this is waiting for JDK 11 to open up.

thanks!
Coleen

>
> regards,
>
>
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
In reply to this post by David Holmes
Thank you, David.

On 12/7/17 10:37 PM, David Holmes wrote:

> On 8/12/2017 1:28 AM, [hidden email] wrote:
>> On 12/7/17 8:11 AM, [hidden email] wrote:
>>>
>>>
>>> On 12/6/17 11:33 PM, David Holmes wrote:
>>>> On 7/12/2017 2:00 PM, [hidden email] wrote:
>>>>> On 12/6/17 9:41 PM, David Holmes wrote:
>>>>>> Hi Coleen,
>>>>>>
>>>>>> On 7/12/2017 7:59 AM, [hidden email] 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
>>>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
In reply to this post by coleen.phillimore
On 12/6/17 4:59 PM, [hidden email] wrote:

> Summary: Change parameter types from jlong, jint, jbyte to int64_t,
> int32_t and int8_t respectively
>
> 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

src/hotspot/cpu/x86/stubGenerator_x86_32.cpp
     No comments.

src/hotspot/cpu/x86/stubGenerator_x86_64.cpp
     No comments.

src/hotspot/cpu/zero/stubGenerator_zero.cpp
     No comments.

src/hotspot/os_cpu/bsd_x86/atomic_bsd_x86.hpp
     No comments.

src/hotspot/os_cpu/bsd_x86/bsd_x86_32.s
     No comments.

src/hotspot/os_cpu/bsd_zero/atomic_bsd_zero.hpp
     No comments.

src/hotspot/os_cpu/linux_arm/atomic_linux_arm.hpp
     No comments.

src/hotspot/os_cpu/linux_arm/os_linux_arm.cpp
     No comments.

src/hotspot/os_cpu/linux_arm/os_linux_arm.hpp
     No comments.

src/hotspot/os_cpu/linux_sparc/os_linux_sparc.hpp
     No comments.

src/hotspot/os_cpu/linux_x86/atomic_linux_x86.hpp
     No comments.

src/hotspot/os_cpu/linux_zero/atomic_linux_zero.hpp
     No comments.

src/hotspot/os_cpu/solaris_sparc/os_solaris_sparc.hpp
     No comments.

src/hotspot/os_cpu/solaris_x86/atomic_solaris_x86.hpp
     No comments.

src/hotspot/os_cpu/solaris_x86/os_solaris_x86.cpp
     No comments.

src/hotspot/os_cpu/solaris_x86/os_solaris_x86.hpp
     No comments.

src/hotspot/os_cpu/windows_x86/atomic_windows_x86.hpp
     No comments.

src/hotspot/os_cpu/windows_x86/os_windows_x86.cpp
     No comments.

src/hotspot/os_cpu/windows_x86/os_windows_x86.hpp
     No comments.

src/hotspot/share/runtime/atomic.hpp
     No comments.

src/hotspot/share/runtime/stubRoutines.cpp
     No comments.

src/hotspot/share/runtime/stubRoutines.hpp
     No comments.


Thumbs up.

I'm also scratching my head on some of the switches to "long" usage,
but it looks like you and David worked that out...

Dan



> bug link https://bugs.openjdk.java.net/browse/JDK-8186903
>
> This change is for JDK 11.
>
> Thanks,
> Coleen
>

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore

Thanks Dan!

On 12/8/17 3:18 PM, Daniel D. Daugherty wrote:

> On 12/6/17 4:59 PM, [hidden email] wrote:
>> Summary: Change parameter types from jlong, jint, jbyte to int64_t,
>> int32_t and int8_t respectively
>>
>> 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
>
> src/hotspot/cpu/x86/stubGenerator_x86_32.cpp
>     No comments.
>
> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp
>     No comments.
>
> src/hotspot/cpu/zero/stubGenerator_zero.cpp
>     No comments.
>
> src/hotspot/os_cpu/bsd_x86/atomic_bsd_x86.hpp
>     No comments.
>
> src/hotspot/os_cpu/bsd_x86/bsd_x86_32.s
>     No comments.
>
> src/hotspot/os_cpu/bsd_zero/atomic_bsd_zero.hpp
>     No comments.
>
> src/hotspot/os_cpu/linux_arm/atomic_linux_arm.hpp
>     No comments.
>
> src/hotspot/os_cpu/linux_arm/os_linux_arm.cpp
>     No comments.
>
> src/hotspot/os_cpu/linux_arm/os_linux_arm.hpp
>     No comments.
>
> src/hotspot/os_cpu/linux_sparc/os_linux_sparc.hpp
>     No comments.
>
> src/hotspot/os_cpu/linux_x86/atomic_linux_x86.hpp
>     No comments.
>
> src/hotspot/os_cpu/linux_zero/atomic_linux_zero.hpp
>     No comments.
>
> src/hotspot/os_cpu/solaris_sparc/os_solaris_sparc.hpp
>     No comments.
>
> src/hotspot/os_cpu/solaris_x86/atomic_solaris_x86.hpp
>     No comments.
>
> src/hotspot/os_cpu/solaris_x86/os_solaris_x86.cpp
>     No comments.
>
> src/hotspot/os_cpu/solaris_x86/os_solaris_x86.hpp
>     No comments.
>
> src/hotspot/os_cpu/windows_x86/atomic_windows_x86.hpp
>     No comments.
>
> src/hotspot/os_cpu/windows_x86/os_windows_x86.cpp
>     No comments.
>
> src/hotspot/os_cpu/windows_x86/os_windows_x86.hpp
>     No comments.
>
> src/hotspot/share/runtime/atomic.hpp
>     No comments.
>
> src/hotspot/share/runtime/stubRoutines.cpp
>     No comments.
>
> src/hotspot/share/runtime/stubRoutines.hpp
>     No comments.
>
>
> Thumbs up.
>
> I'm also scratching my head on some of the switches to "long" usage,
> but it looks like you and David worked that out...
>

The ptr variants were removed except this one.  It should be some 64 bit
entity which for the rest of the functions is currently 'long'. So I
made it match.   It's a further cleanup (as always!)  Now all the ptr
variants are gone.

Coleen

> Dan
>
>
>
>> bug link https://bugs.openjdk.java.net/browse/JDK-8186903
>>
>> This change is for JDK 11.
>>
>> Thanks,
>> Coleen
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Andrew Dinn
In reply to this post by coleen.phillimore
On 08/12/17 12:36, [hidden email] wrote:

> On 12/8/17 6:52 AM, Andrew Dinn wrote:
>> On 06/12/17 21:59, [hidden email] wrote:
>>> Other platforms: could you please test this patch?
>> This built fine on AArch64 and successfully passed basic smoke tests
>> (runs javac and netbeans, passes jtreg tests for j.u.c).
>>
>> I am currently running jcstress tests just to be sure. If you want to
>> wait on the result of that I will let you know the result when they
>> finish (all is ok so far but there is still a long way to go).
>
> Hi Andrew,  Did you also review the change?
>
> There's no rush because this is waiting for JDK 11 to open up.
No, I didn't review the change. I think I will be doing so though
because I saw some failures when running jcstress.

Unfortunately, my AArch64 box fell over before the run completed so I
did nto get a complete set of results. However, I did record the 3
failures shown below.

I'll need to rerun with/without your change just to check whether it
really is the culprit.

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

----- 8< -------- 8< -------- 8< -------- 8< -------- 8< -------- 8< ---
  [FAILED] o.o.j.t.coherence.varHandles.fields.opaque.ShortTest
    (fork: #1, iteration #2, JVM args: [-XX:-TieredCompilation])
  Observed state   Occurrences   Expectation  Interpretation

          -1, -1     2,627,869    ACCEPTABLE  Observers sees both read.

           -1, 0             1     FORBIDDEN  Seeing first read, but not
second: coherence violation.
           0, -1            59    ACCEPTABLE  Observe second read, but
not first: sequential consistency.
            0, 0       133,151    ACCEPTABLE  Default value for the
fields. Allowed to see this: data r...

----- 8< -------- 8< -------- 8< -------- 8< -------- 8< -------- 8< ---
  [FAILED] o.o.j.t.coherence.varHandles.fields.opaque.ShortTest
    (fork: #1, iteration #3, JVM args: [-XX:-TieredCompilation])
  Observed state   Occurrences   Expectation  Interpretation

          -1, -1    15,667,937    ACCEPTABLE  Observers sees both read.

           -1, 0             2     FORBIDDEN  Seeing first read, but not
second: coherence violation.
           0, -1         3,713    ACCEPTABLE  Observe second read, but
not first: sequential consistency.
            0, 0       751,608    ACCEPTABLE  Default value for the
fields. Allowed to see this: data r...

----- 8< -------- 8< -------- 8< -------- 8< -------- 8< -------- 8< ---
  [FAILED] o.o.j.t.coherence.varHandles.fields.opaque.ByteTest
    (fork: #1, iteration #3, JVM args: [-XX:-TieredCompilation])
  Observed state   Occurrences   Expectation  Interpretation

          -1, -1    11,867,040    ACCEPTABLE  Observers sees both read.

           -1, 0             1     FORBIDDEN  Seeing first read, but not
second: coherence violation.
           0, -1           671    ACCEPTABLE  Observe second read, but
not first: sequential consistency.
            0, 0        74,538    ACCEPTABLE  Default value for the
fields. Allowed to see this: data r...

----- 8< -------- 8< -------- 8< -------- 8< -------- 8< -------- 8< ---
Reply | Threaded
Open this post in threaded view
|

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

Andrew Dinn
Hi Coleen,

On 11/12/17 09:20, Andrew Dinn wrote:
> I'll need to rerun with/without your change just to check whether it
> really is the culprit.
The failures I saw were nothing to do with your change. I'll pursue them
separately.

So, the patch builds and runs ok on AArch64 (n.b this is an AArch64
build not an arm build).

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore


On 12/11/17 11:09 AM, Andrew Dinn wrote:

> Hi Coleen,
>
> On 11/12/17 09:20, Andrew Dinn wrote:
>> I'll need to rerun with/without your change just to check whether it
>> really is the culprit.
> The failures I saw were nothing to do with your change. I'll pursue them
> separately.
>
> So, the patch builds and runs ok on AArch64 (n.b this is an AArch64
> build not an arm build).

Great, thank you for running these tests.
Coleen

>
> regards,
>
>
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>