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

[David Holmes]

Hi Leo,

On 11/06/2020 8:47 pm, Leo Korinth wrote:
> 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.

You can't expect all hotspot engineers to subscribe to hotspot-gc-dev to 
have this conversation. Either use a common list, like hotspot-dev (that 
we should all be subscribed to) or else cross-post.

Thanks,
David
-----

> 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