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

[David Holmes]

Hi Leo,

I am in general support of what Kim states below. I also want to see 
this adopted (in a suitable form) for use across the Hotspot code base 
in a consistent way.

I can live without the convenience of operator overloading for 
AtomicValue. :)

Cheers,
David
-----

On 25/06/2020 8:39 pm, Kim Barrett wrote:
>> 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.)
>