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

[Leo Korinth]

On 15/06/2020 14:39, coleen.phillimore at oracle.com wrote:
> 
> I like the concept of this.  I really want the compiler to do to the 
> work to verify that if something is interestingly concurrent, we 
> shouldn't use plain loads and stores of it.
> 
> The name AtomicValue<> seems as good as any.  Can AtomicValue be 
> declared in its own header file: runtime/atomicValue.hpp please?

Yes, I think that is a good idea, and when I move it and revert changes 
in atomics.hpp that Kim disliked, atomics will be untouched I think.

I will incorporate this in the next version, thanks!

> 
> On 6/14/20 4:51 AM, Kim Barrett wrote:
>>> On Jun 10, 2020, at 8:46 AM, Leo Korinth <leo.korinth at oracle.com> 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 SA doesn't really need anything to be volatile. I think they are 
> there to get vmStructs to compile for volatile fields but don't have any 
> meaning for SA since it is used to look at core files or unresponsive 
> VMs.  I don't see the void* cast that you are referring to in the 
> macros, which doesn't mean it's not there.  The SA seems to read these 
> values as their underlying types so if you don't get errors in 
> test/hotspot/jtreg/serviceability/sa, the changes are fine.  Hopefully 
> you won't have to add AtomicValue to SA.

My changes seem to work if they do not fail in any higher tier (>3). The 
cast I was talking about is the (void*) cast in 
GENERATE_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY in vmStructs.hpp.

The macro seems redundant; it was used inconsistently. Volatiles fields 
sometimes uses volatile_nonstatic_field() and sometimes 
nonstatic_field() --- if the latter, passing volatile as part of the 
type argument. Maybe sometime there were plans to do something more than 
(pointlessly?) cast to void*? Or maybe I am just reading the code badly...

Thanks,
Leo

> 
> thanks,
> Coleen
>