RFR: 8186166: Generalize Atomic::cmpxchg with templates
kim.barrett at oracle.com
Mon Aug 14 16:35:37 UTC 2017
> 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.
> 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
> 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
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
> I find that all rather dismaying.
> 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.
> 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:
Native testing needs support for compile-fail tests
More information about the hotspot-dev