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

[Kim Barrett]

> On Jun 24, 2020, at 7:07 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
> 
> 
> 
> 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.

I don't think the primary point of this change should be "eliminating
volatile".  There are places where we're not going to be able to use
this new class that still need "volatile-ish" treatment.  So it should
have the effect of (significantly) reducing the use of volatile for
what is effectively documentation, but I don't see elimination being
possible. 

While the standard doesn't make any guarantees about word tearing for
volatile accesses, there are platform guarantees for word-size or
below and properly aligned and not weird memory cases (in other words,
any case interesting to us).  And indeed, there are lots of hardware
access APIs that would be completely broken otherwise.

And we need to make assumptions along those lines about volatile in
order to implement the Atomic class's APIs, since we're not generally
using "real" atomics provided by the compiler and language. (And for
reasons discussed elsewhere, that seems unlikely to change.)

My view of this change is that it uses the compiler to help keep us
honest about what we're doing, making atomic operations explicit (such
as always needing load/store operations), as well as making
"non-atomic" operations on otherwise atomic variables explicit. For
example, not having increment and decrement operators means we can't
unintentionally perform operations that aren't what we intend.

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

I think operator overloading has benefits in some places, but I think
this is not one of those places. If we were going to overload
operators here I would suggest using the strongest ordering
constraints available, rather than the weakest, which is effectively
what we have today. I think having the syntactically most convenient
form be the least safe (as in the current state) is not a great idea.
And I'm definitely in the camp that prefers atomic operations to stand
out.

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

I think that at this point a substantial fraction (perhaps even the
majority) of the remaining uses of volatile outside GC code are
exactly the use-case for this new class. And I think the remainder are
mostly not removable. (I say "mostly" because there may be some that
actually don't require any special treatment. Though I think the last
of the volatile member qualifiers has been eliminated, and I haven't
found any remaining volatile parameters, which never made sense.)

I think it would be bad to have a bifurcation of GC code dealing with
atomics (mostly) one way and the rest of HotSpot using a different
approach. (Even if we could always criply agree on where the boundary
is.)