RFR (M): 8188224: Generalize Atomic::load/store to use templates
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Wed Oct 4 12:09:55 UTC 2017
On 10/4/17 8:08 AM, coleen.phillimore at oracle.com wrote:
> So this change is becoming more familiar but I think it's because the
> comment is repeated now for cmpxchg, add, and now load and store. My
> scanning ability is too limited to spot the differences. I don't like
> the duplicated comments at all.
^ long (> 1 line)
> I don't know if this is possible and not with this change, but I think
> there should be a class platformAtomic.hpp which consolidates these
> comments and moves the platform* stuff out of atomic.hpp, to be
> included or subclassed by atomic.hpp. Then we can find our desired
> Atomic::blah functions again. I would like an RFE for this.
> Otherwise, I've pattern matched this and it seems correct and am fine
> with checking this in.
> These changes I really like because now we don't have to go hunting to
> see that atomic::load/store is just *thing.
> On 10/3/17 8:58 AM, Erik Österlund wrote:
>> Hi David,
>> Thanks for the review.
>> The comments have been removed.
>> New full webrev:
>> New incremental webrev:
>> On 2017-10-03 14:44, David Holmes wrote:
>>> Hi Erik,
>>> A lot of jumping through hoops just to do a direct load/store in the
>>> bulk of cases - but okay, we're embracing templates.
>>> 66 // Atomically store to a location
>>> 67 // See comment above about using jlong atomics on 32-bit platforms
>>> The comment at #67 and the equivalent one for load can be deleted.
>>> The "comment above" should only be referring to r-m-w atomic ops not
>>> basic load and store. All platforms must have a means to do atomic
>>> load/store of 64-bit due to Java volatile variables (eg by using
>>> floating-point unit on 32-bit) but may not have cmpxchg<8>
>>> capability. (I failed to convince the author of this when those
>>> comments went in. ;-) )
>>> On 3/10/2017 10:29 PM, Erik Österlund wrote:
>>>> The time has come to generalize Atomic::load/store with templates -
>>>> the last operation to generalize in Atomic.
>>>> The design was inspired by Atomic::xchg and uses a similar
>>>> mechanism to validate the passed in arguments. It was also designed
>>>> with coming OrderAccess changes in mind. OrderAccess also contains
>>>> loads and stores that will reuse the LoadImpl and StoreImpl
>>>> infrastructure in Atomic::load/store. (the type checking for what
>>>> is okay to pass in to Atomic::load/store is very much the same for
>>>> One thing worth mentioning is that the bsd zero port (but notably
>>>> not the linux zero port) had a leading fence for atomic stores of
>>>> jint when #if !defined(ARM) && !defined(M68K) is true without any
>>>> comment describing why. So I took the liberty of removing it.
>>>> Atomic should not have any fencing at all - that is what
>>>> OrderAccess is for. In fact Atomic does not promise any memory
>>>> ordering semantics for loads and stores. Atomic merely provides
>>>> relaxed accesses that are atomic. Worth mentioning nevertheless in
>>>> case anyone wants to keep that jint Atomic::store fence on bsd zero
>>>> !M68K && !ARM.
>>>> Testing: JPRT, mach5 hs-tier3
More information about the hotspot-dev