RFR: 8186166: Generalize Atomic::cmpxchg with templates
david.holmes at oracle.com
Mon Aug 14 05:08:23 UTC 2017
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.
First for the record, and for those who followed the other review
thread, I have been learning about template metaprogramming under Kim's
guidance. I can see some uses for it, even though it can be extremely
difficult to read and interpret. I'm not a Functional Programmer so I
don't find the style of it particularly appealing, or "natural". But I'm
no longer opposed to its use in moderation and with good taste - and
with definite advantage to its use. :)
> One goal is to generalize the API and make it more type-safe,
> eliminating (nearly) all need for casts by callers. Another is to
> allow more direct per-platform implementations, especially where there
> are appropriate compiler intrinsics available.
> If all goes well and this change is accepted, it is intended that the
> remainder of Atomic, and later OrderAccess, will be changed in a
> similar manner as one or more followups.
> This change to cmpxchg deprecates cmpxchg_ptr. Followup changes will
> convert existing uses of cmpxchg_ptr and eventually remove it.
> Followup changes will also update uses of cmpxchg that can be
> simplified because of the generalized API.
> The cmpxchg function calls private Atomic::CmpxchgImpl, to select
> among several cases, based on the argument types. Each case performs
> some checking and possibly conversion to reach a standard form of the
> arguments (or fail and report a compile time error), which are then
> passed to private Atomic::PlatformCmpxchg. The PlatformCmpxchg class
> is provided by each platform, and performs additional case analysis to
> generate the appropriate code for the platform.
Not sure how many levels of indirection we have here yet ...
> This change involves modifications to all of the existing platform
> implementations of cmpxchg. I've updated all of the platforms, but
> only tested those supported by Oracle. I'll need help from the
> various platform maintainers to check and test my changes.
> Changes to files can be categorized as follows:
> New metaprogramming utilities:
> Change cmpxchg to a template:
> Oracle-supported platforms:
> atomic_bsd_x86.hpp - 64bit only
> Non-Oracle platform changes:
> atomic_bsd_x86.hpp - 32bit
> Usage changes required by API change:
> Example updates of cmpxchg_ptr to cmpxchg:
> Some specific issues:
> - For Solaris (both SPARC and x86), we were using helper functions
> defined in corresponding .il files. Using the cmpxchg_using_stub
> helper didn't work; passing a "defined in .il" function as a template
> parameter didn't work, leading to linker errors. The ideal solution
> is to use gcc-style inline assembly, which is now supported by Solaris
> Studio. That change was made for SPARC. However, making that change
> for x86 ran into related segfaults, so using the .il definition with
> direct conversions for now, until that problem can be resolved.
We already have an issue open for getting rid of the .il files:
The template usage probably adds fuel to the argument for doing so.
> - 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.
> - 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?
> - 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
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.)
> * Conversions between integers and floating point values are presently
> implemented using the memcpy technique. While this technique is
> correct, it is known to be quite poorly optimized by some compilers.
> It may be necessary to use the "union trick" for performance reasons,
> even though it is technically undefined behavior. Hotspot already
> does this elsewhere (and we should merge some of the different
> conversion handling).
More argument for just avoiding this morass if we don't need to go there.
> * In the original RFR for JDK-8184334, conversions between different
> integral types of the same size (along with enums) were performed
> This was based on discussion from several months ago, referring to
> C++03 3.10/15. However, re-reading that section in response to
> discussion of the RFR for JDK-8184334, I now think that implementation
> is not supported in many cases needed here, and indeed invokes
> undefined behavior in those cases. In particular, where 3.10/15 says
> "- a type that is the signed or unsigned type corresponding to the
> dynamic type of the object,"
> I interpret this to mean that if the dynamic type of an object is an
> integral type of a given rank, the value may be accessed using an
> lvalue of an integral type of the same rank and the opposite sign.
> (Using the same rank and sign is already permitted, since that's the
> same type.) I don't think it allows access using an lvalue of an
> integral type with a different rank. So, for example, a signed int
> value may be accessed via an unsigned int lvalue. But access via a
> signed long lvalue is not permitted, even if int and long have the
> same size.
> I don't think there is a good way to perform the integral and enum
> conversions we need, without invoking implementation-defined,
> unspecified, or undefined behavior.
> ** The memcpy technique avoids such problems, but as noted above, is
> known to be quite poorly optimized by some compilers.
> ** Using static_cast avoids any undefined behavior, but does have
> cases of implementation-defined or unspecified behavior. (When
> casting an unsigned value to a signed type, the result may be
> implementation defined; 5.2.9/2 and 4.7/3. C++11 <type_traits> plus
> the reinterpret_cast to reference seems to provide possibly multi-step
> conversion paths that avoid such qualifications on all of the steps.)
> (When casting an integral value to an enum type, the result may be
> unspecified; 5.2.9/7. With C++11 <type_traits>, the problem doesn't
> arise so long as the starting value is appropriate, e.g. we're
> reversing an enum to integer conversion.)
> ** Again here, we might consider the "union trick".
> I haven't changed the implementation from the JDK-8184334 RFR, but I'm
> presently leaning toward the "union trick", despite previous strong
> resistance to it. Right now it seems to me to be the best option in
> practice, despite being disallowed by the standard. It's really too
> bad the memcpy technique is so poorly supported by some compilers.
I find that all rather dismaying.
Looking at the new metaprogramming files first:
With a bit of exertion I can read all this and am 90% confident I
understand what they do.
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?
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 ... ?
That's all for now.
> Ad hoc Hotspot nightly test.
More information about the hotspot-dev