RFR: 8247213: G1: Reduce usage of volatile in favour of Atomic operations

[David Holmes]

Hi Leo,

Thanks for undertaking this effort!

Some higher-level comments at this stage just looking at the 
introduction of AtomicValue.

As Kim has pointed out it may not always be appropriate to change all 
fields subject to atomic operations to be converted to AtomicValue. 
There is also the issue of fields accessed in a mix of locked and 
lock-free mechanisms (particularly in the runtime where we have locked 
writes and lock-free reads in a number of places, though sometimes 
combined with atomic updates). If such fields are converted to 
AtomicValue then I think we may still need a way to perform some raw 
operations on them; or else we chose to leave them as currently defined 
and apply Atomic ops directly when needed. (There is a parallel in the 
Java APIs where we can declare a field as an AtomicX variable, or else 
we can declare the field simply as X, and use an AtomicFieldUpdater to 
perform the atomic operations.)

I like to imagine AtomicValue as a library-based alternative to a true 
"atomic" keyword in a language, but I'm torn between conciseness of 
expression and clarity of action. If we had an atomic keyword then:

atomic int x = 0;
...
x = 3;  // this is an atomic store
x++;    // this is an atomic post-increment

which would then argue for:

AtomicValue<int> x = 0;
...
x = 3;  // this is an atomic store
x++;    // this is an atomic post-increment

rather than the explicit

x.store(3);  // this is an atomic store
x.inc();     // this is an atomic post-increment

in particular having explicit load() and store() operations makes for 
very unwieldy expressions IMHO.

That said I know some people prefer it if atomic operations stand out 
very clearly.

So I think we need to get a general consensus on what shape AtomicValue 
should take and exactly how we propose to use it (or not) across all of 
hotspot.

And perhaps, as Kim alluded, the addition of AtomicValue should be split 
out from its application to any particular subsystem.

Thanks,
David
-----

On 12/06/2020 1:51 am, Leo Korinth wrote:
>>> Hi, could I have a review for this change that adds AtomicValue<> to 
>>> atomic.hpp and uses it in G1?
>>>
>>> I am adding an AtomicValue<> to atomic.hpp. This is an opaque type 
>>> (the "value" is private) and that protect against non-atomic 
>>> operations being used by mistake. AtomicValue methods are proxied to 
>>> the corresponding (all-static) Atomic:: methods. All operations are 
>>> explicitly atomic and in a type safe manner with semantics defined in 
>>> enum atomic_memory_order
>>> (memory_order_conservative by default).
>>>
>>> Instead of using field variables as volatile, I change them to be of 
>>> AtomicValue<> type. No need to verify that += (for example) will 
>>> result in an atomic instruction on all supported compilers.
>>>
>>> I have some open questions regarding the exported fields in 
>>> vmStructs_g1.hpp. Today, volatile fields are sometimes "exported" as 
>>> volatile (and sometimes not, and I guess this is by mistake). I 
>>> choose to export them all as non-volatile. From what I can see the 
>>> volatile specific part only casts to void* (only documentation?). 
>>> Java code is unchanged and still access them as the unwrapped values 
>>> (static assert in atomic.hpp guarantees that memory layout is the 
>>> same for T and AtomicValue<T>). I think this is okay, but would like 
>>> feedback on all this.
>>>
>>> The change is presented as a two part change. The first part changes 
>>> all volatile to AtomicValue, the other part removes the AtomicValue 
>>> part on non-field accesses. By doing it two part I will not forget to 
>>> transform some operations by mistake.
>>>
>>> Copyright years will be updated when all other changes are approved.
>>>
>>> How about pushing this after 15 is branched off and thus have it for 16?
>>>
>>> Enhancement:
>>> https://bugs.openjdk.java.net/browse/JDK-8247213
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~lkorinth/8247213/0/part1
>>> http://cr.openjdk.java.net/~lkorinth/8247213/0/part2
>>> http://cr.openjdk.java.net/~lkorinth/8247213/0/full
>>>
>>> Testing:
>>>    tier1-3
>>>
>>> Thanks,
>>> Leo
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Thanks,
>>>>> Leo