RFR: 8186476: Generalize Atomic::add with templates

Tue Aug 29 07:13:12 UTC 2017

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:
>>>> On 28 Aug 2017, at 14:46, coleen.phillimore at oracle.com wrote:
>>>> http://cr.openjdk.java.net/~kbarrett/8186476/hotspot.00/src/share/vm/runtime/atomic.hpp.udiff.html 
>>>>
>>>> Can Atomic::{inc,dec}(volatile jshort* dest) use u2 instead?  And 
>>>> the jshort version of Atomic::add use u2 instead of jshort?   Can 
>>>> it be done as a follow on change, since I thought we were trying to 
>>>> remove the jtypes from this runtime internal code?
>>> Yes. I will file an RFE for that.
>>
>> Thanks!
>>>
>>>> I was confused, I thought you'd do Atomic::inc() in this change, 
>>>> but I'm in all in favor of limiting changes like this.
>>> I have worked on Atomic::inc/dec in a follow on RFE that I am 
>>> currently running through testing.
>>>
>>>> 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.

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