RFR (M): 8184334: Generalizing Atomic with templates

[coleen.phillimore at oracle.com]

Thank you to Kim for being patient and explaining metaprogramming 101 to me.

This latest version seems much simpler and less like "line noise". It's 
still very dense.  Some lines have implicit compiler actions that you 
pretty much just have to know about.   I asked Kim to put a few more 
comments in places to quickly answer the question, essentially, "why do 
we need this?"

What I like about this is that it has exactly the code needed to 
cast/transform template types to the types to the sizes that the 
platforms support.  I think it isolates the casts to one place, in the 
template layer, with checking for compatible sizes.  We were talking 
about this and being able to add more checking in the template layer, 
eg. that the template destination parameter is declared volatile, which 
is easy to forget.

I think with this code, we can incrementally clean up the rest of the 
code to study and eliminate casts.   Andrew said that lock free 
programming is tricky to write, but I'd rather not spend time on missed 
return type casts or blindly copied intptr_t casts rather concentrate on 
the multiprogramming aspect of the code.

 From what I understand, the flexibility of template code is necessary 
for the GC interface, which we all want, and the removal of casting is a 
bonus.

On 8/6/17 7:32 PM, 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.
>
> 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.

This solution seems pretty understandable to me after learning what a 
TrueType and FalseType are.    See 
src/share/vm/metaprogramming/integralConstant.hpp

  Example:
http://cr.openjdk.java.net/~kbarrett/cmpxchg_template_20170806/webrev/src/share/vm/oops/oopsHierarchy.hpp.udiff.html

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

This seems pretty understandable to me also, example:
http://cr.openjdk.java.net/~kbarrett/cmpxchg_template_20170806/webrev/src/share/vm/aot/aotCodeHeap.hpp.udiff.html
>
> 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.
>
Besides some helpful comment suggestions, I think this looks good to 
build on.   The bitMap changes should be motivating.

thanks,
Coleen