RFR: 8186476: Generalize Atomic::add with templates

David Holmes david.holmes at oracle.com
Tue Aug 29 07:28:46 UTC 2017 

<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;
};

???

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

More information about the hotspot-dev mailing list