RFR: 8186166: Generalize Atomic::cmpxchg with templates

Tue Aug 15 06:57:19 UTC 2017

Hi Kim,

All points in your response below noted - in particular the problem with 
enum definition locations for registration.

Again many thanks for your perseverance on this and for making things 
more palatable for those of us not familiar with this style of programming.

Here's the rest of the review - with some things already addressed, or 
flagged, by Coleen's review and your responses ("sizeof trick"!!). Many 
of my initial questions/comments have resolved themselves as I've gotten 
further through the review. :) Just a few queries and a couple of 
suggestions left.

src/os_cpu/aix_ppc/vm/atomic_aix_ppc.hpp

The actual body has:

435   long old_value;

rather than "T old_value". I presume the AIX compiler can't handle using 
T with inline asm?

---

src/os_cpu/windows_x86/vm/atomic_windows_x86.hpp

We must revisit why we are using the stub generator on Windows!

---

src/share/vm/gc/shared/workgroup.cpp

     if (old == 0) {
!     old = Atomic::cmpxchg(1u, &_tasks[t], 0u);
     }
     assert(_tasks[t] == 1, "What else?");

I don't understand why we can compare against 0 and 1, yet must pass 0u 
and 1u to our extremely clever template function. :(

---

src/share/vm/oops/oop.inline.hpp

400                                          volatile HeapWord *dest,
411     narrowOop old = Atomic::cmpxchg(val, (narrowOop*)dest, cmp);
418     return Atomic::cmpxchg(exchange_value, (oop*)dest, compare_value);

do the casts here indicate we should have some conversion function from 
HeapWord to oop and narrowoop?

---

src/share/vm/runtime/atomic.hpp

So the overall chain of things is:

cmpxchg -> CmpxchgImpl -> PlatformCmpxchg<N> ->  inline assembly; or
                                             |-> cmpxchg_using_stub -> 
external .s/.il definition.

I'm unclear why we need CmpxchgImpl - or more accurately I'm unclear why 
all the type "dispatching" that CmpxchgImpl does can't be done by the 
top-level Atomic::cmpxchg template?

So cmpxchg_using_stub exists to deal with any needed conversions (type 
or order) between the PlatformCmpxchg API and the actual 
platform-specific functions (if they exist) - ok. Only nit I have here 
is the use of "stub" because when I see that I think of the 
stubGenerator, and this has nothing to do with that. How about 
cmpxchg_adapter? (And perhaps StubFn should be something else ... I 
don't see it as a stub, as its the real function to do the work ... 
could just be Fn?

---

inline T Atomic::PlatformCmpxchg<1>::operator()

Coleen flagged that use of operator() is somewhat obscure, and I tend to 
agree. At the moment calling it cmpxchg might seem redundant, but what 
if the naming scheme were eg:

Atomic::Platform<1>:cmpxchg
Atomic::Platform<1>:add
etc?

---

  182   template<typename StubType, typename StubFn, typename T>
  183   static T cmpxchg_using_stub(StubFn stub_fn,
  184                               T exchange_value,
  185                               T volatile* dest,
  186                               T compare_value);

Templates 101 question: why when we instantiate this template do we only 
need to pass StubType explicitly eg:

cmpxchg_using_stub<jlong>(_Atomic_cmpxchg_long, exchange_value, dest, 
compare_value);

are the other types inferred from the function arguments?

---

IsPointerConvertible ... yep now I get it. I got the sizeof trick 
quickly but was missing the actual conversion check - now realizing that 
test(<value of type From*>) will select test(To*) if there is a 
conversion, and test(...) if not. :)

----

That's it. :)

Thanks,
David
-----

On 15/08/2017 2:35 AM, Kim Barrett wrote:
>> On Aug 14, 2017, at 1:08 AM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Hi Kim,
>>
>> This is an initial partial review. I have to move onto other stuff today so will get back to this tomorrow.
> 
> Thanks.
> 
>> On 14/08/2017 12:41 PM, Kim Barrett wrote:
>>> Please review this change to Atomic::cmpxchg, making it a function
>>> template with a per-platform underlying implementation.
>>> This has been broken out from 8184334: "Generalizing Atomic with
>>> templates", as the originally proposed change there encountered
>>> various objections.  This change takes a somewhat different approach
>>> to the same problem.
>>
>> I assume the intent is not to push this yet, so that Erik has a chance to weigh-in when he gets back from vacation? I would not want to see unnecessary churn here.
> 
> I had thought it might depend on how quickly this review went.  But
> Jesper's request to make jdk/hs temporarily slushy makes a push before
> Erik's return seem quite unlikely.
> 
>> […]
>>> - I've added the function template Atomic::condition_store_ptr, which
>>> is a cmpxchg for pointer types with a NULL compare_value.  That might
>>> not be the best name for it.
>>
>> Definitely not the best name for it. :) replace_if_null seems to describe it more precisely.
> 
> That seems like a good name.  I'll wait a bit for other opinions.
> 
>>> - The name IntegerTypes was retained from the original RFR for
>>> JDK-8184334.  However, that name no longer fits the provided
>>> functionality very well, and I think a new name is needed.  Something
>>> involving Bitwise, or ValueRepresentation perhaps?  I'm open to
>>> suggestions.
>>
>> How about PrimitiveTypes?
> 
> I'll wait for other opinions.  That name suggests to me that support
> for floating point should be included.  And see below.
> 
>>> - Issues with casts/conversions provided by IntegerTypes.
>>> * Conversions between integers and floating point values are
>>> supported.  It was suggested during the discussion of the RFR for
>>> JDK-8184334 that this was not needed, as there are no uses of
>>> atomic/ordered floating point values in Hotspot.  However, I'm told
>>> VarHandles may include atomic floating point values.
>>
>> Yes they were added. However at the level of jdk.internal.misc.Unsafe all the FP operations are converted to int/long variants using eg Float.floatToRawIntBits etc.
>>
>> But I'm not convinced we need to complicate things by supporting floating-point Atomic::cmpxchg in the VM's Atomic API. You can only do so by implementing the floatToRawIntBits etc functionality, and there are no native mappings for any "atomic" floating-point operation (ie the only way to do Atomic::inc on a floating=point value is to use cmpxchg.)
> 
> I was about to agree to removal of floating point from here.  Then I
> remembered the jmumble_cast suite of functions in
> globalDefinitions.hpp.  Erik and I had discussed a unification, with
> the jmumble_cast functions built on IntegerTypes.  We can't presently
> do that, because of include dependencies, but that's probably fixable.
> We see this utility as having application beyond Atomic &etc template
> reimplementation.
> 
> But clearly the memcpy technique can't be retained, due to the
> deficiencies of some compilers.
> 
>>> * In the original RFR for JDK-8184334, conversions between different
>>> integral types of the same size (along with enums) were performed
>>> using
>>>    reinterpret_cast<ToType&>(from_value)
>>> […]
>>
>> I find that all rather dismaying.
> 
> No kidding!
> 
>> […]
>> src/share/vm/metaprogramming/isRegisteredEnum.hpp
>>
>> My initial personal preference here is to see registered enum types added to this header file rather than as template specializations at the point of use. Is that reasonable/feasible, or are there reasons to spread this around?
> 
> That would require moving the complete enum definitions here, far away
> from their natural location.  We can't forward declare them here for
> registration; forward declaration of enum types doesn't work (doesn't
> compile), since the compiler can't determine the underlying type
> without knowing the enumerators.  Also, such a move would only be
> possible for an enum type defined at namespace scope; class scoped
> enums cannot be so moved.
> 
>> test/native/metaprogramming/test_integerTypes.cpp
>> test/native/metaprogramming/test_isRegisteredEnum.cpp
>>
>> Seems minimalist but ok.
>>
>> Aside: I find it slightly limiting that because these templates involve compile-time checks, that we can't actually write negative tests. Seems to me that when writing the templates you had to write a test case that will fail, to verify the correctness of the template, but you can't check-in that testcase. Maybe we need a test harness that can handle 'tests' that simply fail to compile ... ?
> 
> When the addition of native testing was being discussed, that was a
> feature I mentioned as being needed.  But that doesn't seem to have
> happened, and I couldn't find an existing RFE, so I filed a new one:
> https://bugs.openjdk.java.net/browse/JDK-8186177
> Native testing needs support for compile-fail tests
> 