RFR: 8186476: Generalize Atomic::add with templates

Tue Aug 29 08:00:44 UTC 2017

Hi David,

On 2017-08-29 09:28, David Holmes wrote:
> <trimming>
>
> On 29/08/2017 5:13 PM, Erik Österlund wrote:
>> Hi Coleen, David,
>>
>> On 2017-08-29 09:05, David Holmes wrote:
>>> On 29/08/2017 5:33 AM, coleen.phillimore at oracle.com wrote:
>>>> On 8/28/17 12:33 PM, Erik Osterlund wrote:
>>>>>> What is the distinction between FetchAndAdd and AddAndFetch?   
>>>>>> Does one assume that the value is in a register and is based on 
>>>>>> the underlying platform implementation?  Can you put a comment in 
>>>>>> atomic.hpp why one vs the other?  These platform inconsistencies 
>>>>>> are really painful since nobody can't test all the platforms.   I 
>>>>>> don't need to see another version of this change, the rest looks 
>>>>>> ok to me.
>>>>> The distinction is whether the platform specialization returns the 
>>>>> value before or after the add, i.e., whether whether the returned 
>>>>> fetched value corresponds to the old or new value, as indicated by 
>>>>> the corresponding name. This is a standard convention used by e.g. 
>>>>> GCC intrinsics.
>>>>
>>>> Oh, I see.  Since nothing generally uses the return value, the 
>>>> inconsistency doesn't matter?
>>>
>>> The Atomic::add API specifies that the returned value is the updated 
>>> value. I must confess I'm now a bit confused here as it seems to me 
>>> that PlatformAdd should always be a "AddAndFetch" to ensure the 
>>> correct semantics. ???
>>
>> Yes, you are right that the public API uses add_and_fetch semantics. 
>> But AddAndFetch and FetchAndAdd are only adapters to the platform 
>> layer for internal use. The platform layer has a specialization that 
>> is specified as either FetchAndAdd or AddAndFetch, which is converted 
>> to the intended add_and_fetch semantics expected by the public API in 
>> Atomic::FetchAndAdd<Derived>::operator() and 
>> Atomic::AddAndFetch<Derived>::operator() correspondingly. So it is 
>> only a helper for the platform layer.
>
> Okay I'm completely lost here - sorry. I'm expecting to see a 
> PlatformAdd that has the semantics of Atomic::add - which does an 
> add_and_fetch. So this looks fine:
>
> src/os_cpu/linux_sparc/vm/atomic_linux_sparc.hpp
>
> template<size_t byte_size>
> struct Atomic::PlatformAdd
>   : Atomic::AddAndFetch<Atomic::PlatformAdd<byte_size> >
> {
>   template<typename I, typename D>
>   D add_and_fetch(I add_value, D volatile* dest) const;
> };
>
> while this looks wrong:
>
> src/os_cpu/linux_x86/vm/atomic_linux_x86.hpp
>
> template<size_t byte_size>
> struct Atomic::PlatformAdd
>   : Atomic::FetchAndAdd<Atomic::PlatformAdd<byte_size> >
> {
>   template<typename I, typename D>
>   D fetch_and_add(I add_value, D volatile* dest) const;
> };
>
> ???

I will try to describe how this work in a hopefully understandable way.

1) Atomic::add calls AddImpl::operator().
2) AddImpl calls PlatformAdd::operator() that could override to do 
anything, but for convenience typically uses either the FetchAndAdd or 
AddAndFetch helper (depending on whether your platform implementation is 
best expressed as a fetch_and_add or add_and_fetch)
3.a) FetchAndAdd::operator() (in shared code) calls 
PlatformAdd::fetch_and_add() (in the platform layer) and adds the addend 
to turn it into add_and_fetch semantics as expected by the public API.
3.b) AddAndFetch::operator() (in shared code) calls 
PlatformAdd::add_and_fetch() (in the platform layer) without adding the 
addend as the semantics in the platform layer already matches the 
expected semantics of the public API.

The key point here is that the platform specified fetch_and_add or 
add_and_fetch member functions are not called directly from 
AddImpl::operator(), they are called through a helper e.g. 
FetchAndAdd::operator() that performs the necessary translation of the 
platform provided intrinsic (in either of the two conventions) to the 
expected shared API semantics. And of course, the shared API has the 
exact same semantics on all platforms.

Hope this makes sense.

Thanks,
/Erik

>
> Thanks,
> David
>
>> Thanks,
>> /Erik
>>
>>> David
>>> -----
>>>
>>>
>>>>>
>>>>> Unfortunately I have already pushed the change as David and Andrew 
>>>>> said I was good to go. I am sorry about that. I will add a short 
>>>>> comment in a subsequent inc/dec patch for you.
>>>>
>>>> Okay.
>>>> Coleen
>>>>
>>>>> Thanks for the review.
>>>>>
>>>>> Thanks,
>>>>> /Erik
>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>>
>>>>>>
>>>>>>> On 8/20/17 2:16 AM, Kim Barrett wrote:
>>>>>>> Please review this change to Atomic::add, making it a function
>>>>>>> template with a per-platform underlying implementation.
>>>>>>>
>>>>>>> This change is similar to 8186166: "Generalize Atomic::cmpxchg with
>>>>>>> templates", with similar goals and taking a similar approach.
>>>>>>>
>>>>>>> This change deprecates add_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.  Only changes to usage that were
>>>>>>> required by the new implementation have been made so far.
>>>>>>>
>>>>>>> The add function calls private Atomic::AddImpl, 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::PlatformAdd.
>>>>>>>
>>>>>>> Only safe conversions of the first (add_value) argument are made by
>>>>>>> the AddImpl layer.  add_value must be integral, and is never 
>>>>>>> narrowed.
>>>>>>> dest's value type must be either integral or a pointer type.  When
>>>>>>> both are integral, they must be of the same signedness. If smaller,
>>>>>>> add_value will be promited to the same size as dest's value type.
>>>>>>>
>>>>>>> The PlatformAdd class is provided by each platform, and performs
>>>>>>> additional case analysis to generate the appropriate code for the
>>>>>>> platform.  Most PlatformAdd implementations use one of two 
>>>>>>> helper base
>>>>>>> classes, Atomic::FetchAndAdd and Atomic::AddAndFetch. These provide
>>>>>>> common parts of the implementation in terms of a support function
>>>>>>> provided by the derived PlatformAdd, accessed using CRTP.
>>>>>>>
>>>>>>> This change involves modifications to all of the existing platform
>>>>>>> implementations of Atomic::add.  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:
>>>>>>>
>>>>>>> [No new metaprogramming tools!]
>>>>>>>
>>>>>>> Change add to a template:
>>>>>>> atomic.hpp
>>>>>>>
>>>>>>> Oracle-supported platforms:
>>>>>>> atomic_bsd_x86.hpp - 64bit only
>>>>>>> atomic_linux_arm.hpp
>>>>>>> atomic_linux_x86.hpp
>>>>>>> atomic_solaris_sparc.hpp
>>>>>>> solaris_sparc.il
>>>>>>> atomic_solaris_x86.hpp
>>>>>>> atomic_windows_x86
>>>>>>>
>>>>>>> Non-Oracle platform changes:
>>>>>>> atomic_aix_ppc.hpp
>>>>>>> atomic_bsd_x86.hpp - 32bit
>>>>>>> atomic_bsd_zero.hpp
>>>>>>> atomic_linux_aarch.hpp
>>>>>>> atomic_linux_ppc.hpp
>>>>>>> atomic_linux_s390.hpp
>>>>>>> atomic_linux_sparc.hpp
>>>>>>> atomic_linux_zero.hpp
>>>>>>>
>>>>>>> Usage changes required by API change:
>>>>>>> g1CardLiveData.cpp
>>>>>>> g1ConcurrentMark.cpp
>>>>>>> g1HotCardCache.cpp
>>>>>>> g1HotCardCache.hpp
>>>>>>> g1RemSet.cpp
>>>>>>> symbol.cpp
>>>>>>> mallocTracker.hpp
>>>>>>>
>>>>>>> A few specific items:
>>>>>>>
>>>>>>> - atomic_linux_arm.hpp
>>>>>>>
>>>>>>> Neither add variant has "cc" in the clobbers list, even though the
>>>>>>> condition codes are modified.  That seems like a pre-existing bug.
>>>>>>>
>>>>>>> - atomic_linux_sparc.hpp
>>>>>>>
>>>>>>> Neither add variant has "cc" in the clobbers list, even though the
>>>>>>> condition codes are modified.  That seems like a pre-existing bug.
>>>>>>>
>>>>>>> The 32-bit add was using intptr_t as the rv type. Probably a 
>>>>>>> harmless
>>>>>>> copy-paste mistake.  Now using template parameter type.
>>>>>>>
>>>>>>> Uses hard-coded machine registers and assumes the inline-asm
>>>>>>> processing assigns the values to the corresponding machine 
>>>>>>> registers,
>>>>>>> even though the given constraints are just generic register 
>>>>>>> requests.
>>>>>>> I don't understand how this can work.
>>>>>>>
>>>>>>> - atomic_solaris_sparc.hpp
>>>>>>>
>>>>>>> Atomic::add was implemented using asm code in solaris_sparc.il.  
>>>>>>> I was
>>>>>>> going to change it to use gcc-style inline-asm, but the linux_sparc
>>>>>>> version wasn't helpful this time (unlike for cmpxchg) (see above).
>>>>>>> Instead, I wrote the CAS-loop in C++, using Atomic::cmpxchg; much
>>>>>>> simpler code, and should be no less efficient, assuming the 
>>>>>>> compiler
>>>>>>> does it's job.
>>>>>>>
>>>>>>> CR:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8186476
>>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~kbarrett/8186476/hotspot.00/
>>>>>>>
>>>>>>> Testing:
>>>>>>> Ad hoc hotspot nightly test.
>>>>>>>
>>>>>>>
>>>>
>>