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

[Leo Korinth]

On 15/06/2020 06:00, David Holmes wrote:
> 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.)

Yes, I am not trying to remove usage of the AllStatic Atomic API, I am 
trying to remove the usage of volatile. Where performance is more 
important than type safety (or if it feels awkward), just continue use 
the Atomic API (see part2 of the change), but I see /no/ need for using 
volatile. No Atomic operations need the volatile qualifier, and (if I 
understand correctly) volatile is generally not what you want as it does 
not guarantee enough from the language for much anything outside writing 
to IO mapped devices.

However when I started to remove volatile in gc code that /mostly/ works 
as documentation, I was afraid of changing the behaviour of the code. 
Using AtomicValue is a great way to do refactoring code and conserve 
behaviour. The two part conversion (part1 and part2) of my change shows 
how AtomicValue is a great way to remove volatile (part1), and in part2 
revert to use the AllStatic Atomic API in certain places without 
forgetting converting implicit volatile load and stores or continuing to 
use non-atomic operations such as ++ on variables (now not volatile) by 
mistake.

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

Many also dislike operator overloading (although I personally like it, I 
think I am in a minority), if nothing else, I believe it hard to get 
through review. I /do/ like that the current verbose explicit way looks 
almost the same as the Atomic API we already have. Implicit load and 
stores also would "look" different from what we have today. I am unsure 
about what I really prefer, and keeping the look seems good and my 
thought was that it would offend the least...

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

I do not want to push others to use it all over hotspot. Maybe not have 
a policy, but use it if it makes sense? Maybe just move it out of 
atomics.hpp as Coleen suggest is enough?

My thinking is that we should reduce usage of volatile all over hotspot 
(regardless of AtomicValue); I do not want AtomicValue to be a forced 
way though.

Thanks,
Leo


> 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