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

[Leo Korinth]

On 11/06/2020 15:34, David Holmes wrote:
> 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.

I realize that now --- sorry --- please continue here on hotspot-dev. I 
closed the thread on hotspot-gc-dev.

/Leo
> 
> 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