RFR (M): 8184334: Generalizing Atomic with templates
david.holmes at oracle.com
Mon Aug 7 01:18:46 UTC 2017
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.
> 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. :(
> 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:
> 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:
> Changed files for uses, so atomic changes compile:
> [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
> I'm looking for feedback on this before I try to carry it any further.
More information about the hotspot-dev