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

[Leo Korinth]

On 11/06/2020 04:58, David Holmes wrote:
> Hi Leo,
> 
> On 10/06/2020 11:02 pm, Leo Korinth wrote:
>> Hi, I have created a RFR in the hotspot-gc-dev mailing list that 
>> mostly changes stuff in G1. However I am also doing changes in 
>> atomic.hpp (mostly adding an AtomicValue<> class) so there might be 
>> interest outside gc. If you have interest, please follow that thread.
> 
> If you are changing atomic.hpp then please cross-post to 
> hotspot-runtime-dev. Or even cross-post here on hotspot-dev as there 
> should be a broad consensus on how to handle this in the VM.

That was what I tried to do by sending the email. I want to have the 
discussion in one place though, so I thought it was better announcing 
the RFR on hotspot-gc-dev on this mailing list than potentially creating 
two review threads by cross-posting.

Next time I will cross post and tell people to reply on hotspot-gc-dev 
if that is preferable. Is it? I have attached the email below:


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