RFR (M): 8184334: Generalizing Atomic with templates

[David Holmes]

Hi Kim,

On 7/08/2017 9:32 AM, Kim Barrett wrote:
> Here is a start at addressing the various comments.  I still owe email
> responses to some comments, but wanted to get this out first.
> 
> http://cr.openjdk.java.net/~kbarrett/cmpxchg_template_20170806/webrev/
> 
> So far this only deals with cmpxchg, and then only for linux-x86/64.
> It builds and passes basic tests.  I want to get feedback on this
> before spending time on other platforms, other operations, and more
> extensive testing.

Pardon my ignorance but I can't follow/read/understand the vast majority 
of this template code. If I was using this API and got a compile-time 
error I likely would not have a clue how to try and figure out what was 
going wrong where. I certainly can not maintain or debug this code, nor 
explain to someone else how it works. :(

David
-----


> The metaprogramming support layer has been substantially reduced and
> simplified.  IntegerTypes is still there, but is more focused, with a
> very small public API.  It also badly needs a new name, but I'll worry
> about that later.
> 
> Floating point types are still there; they can be dropped easily
> enough (and reinstated if later needed).  I've kept them for now
> because they are easy (which is itself somewhat interesting), and
> because I don't recall whether later GC interface work needed it.
> 
> I've included a mechanism for dealing with thin wrappers over
> primitive types.  The driver for this is oop, which is normally just a
> typedef for oopDesc*, but is a class with an oopDesc* member when
> CHECK_UNHANDLED_OOPS is defined (such as during fastdebug builds).  We
> (Erik and I) knew we would need something there, if only for the case
> of oop, but hadn't agreed on a solution.  Well, I'm proposing one here.
> 
> I've included a mechanism for dealing with enums.  The previously
> proposed change didn't handle them, as recognizing enums requires a
> lot more metaprogramming, since we don't have C++11 std::is_enum.  And
> as mentioned earlier, I now think we need to try harder in this area.
> The approach being taken is the registration mechanism I mentioned in
> earlier email.  The reason for including this is to allow filtering
> and dispatching involving enum types, which are *not* integral types,
> even though conversions are supported.
> 
> New files for metaprogramming:
> metaprogramming/integerTypes.hpp
> metaprogramming/registeredEnum.hpp
> 
> Atomic::cmpxchg is still changed to be function template.  And it
> still has three different template parameters.  But there are more
> constraints on the parameters, encoded in the specializations for
> CmpxchgImpl.  (Note that CmpxchgImpl isn't necessary; we could
> accomplish the same dispatch via SFINAEed specializations for
> cmpxchg.  I just prefer the class vs the (IMO) syntactically horried
> syntax for SFINAE of function templates.  C++11 improves that a lot.
> Erik prefers SFINAE of function templates rather than introducing a
> helper class template like CmpxchgImpl.)
> 
> That front-end uses private Atomic::PlatformCmpxchg<size_t>
> to perform .  Specializations are function objects
> with a template operator() with signature T(T, T volatile*, T, order).
> Only Linux x86/64 updated so far.  That makes use of the existing
> inline assembly, but wrapped in templates rather than functions with
> fixed parameter types.  Replacing the assembly code with calls to
> gcc's __sync_compare_and_swap would be syntactically trivial (and
> indeed builds without any problems).
> 
> I've also added bool Atomic::conditional_store_ptr(T, D volatile*),
> for the idiom of storing a value if the old value is NULL.  It turns
> out there are about 25 occurrences of this idiom in Hotspot, so a
> utility for it seems warranted.  The current implementation is just a
> straightforward wrapper around cmpxchg, which means it can't take
> advantage of gcc's __sync_bool_compare_and_swap.  That can be dealt
> with later if desired.
> 
> I also had to modify a few uses of cmpxchg to get this to compile.
> These are presumably the same ones that Andrew encountered.
> 
> Changed files for Atomic:
> runtime/atomic.hpp
> os_cpu/linux_x86/vm/atomic_linux_x86.hpp
> 
> Changed files for uses, so atomic changes compile:
> aot/aotCodeHeap.cpp
> aot/aotCodeHeap.hpp
> gc/parallel/psParallelCompact.hpp
> gc/shared/workgroup.cpp
> runtime/os.cpp
> [Plus one additional closed change.]
> 
> I then replaced a few uses of cmpxchg_ptr with cmpxchg, taking
> advantage of the new API.  This eliminated a number of casts.  There
> are still about 110-120 uses of cmpxchg_ptr remaining.
> 
> Changed files for cmpxchg_ptr removal:
> oops/oop.inline.hpp  -- demonstrates oop translation
> utilities/bitMap.cpp
> utilities/bitMap.inline.hpp
> 
> I'm looking for feedback on this before I try to carry it any further.
>