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

Leo Korinth leo.korinth at oracle.com
Thu Jun 11 12:39:27 UTC 2020


Lets take this review on hotspot-dev instead. Sorry for the confusion.

Thanks,
Leo

On 10/06/2020 14:46, Leo Korinth 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 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



More information about the hotspot-gc-dev mailing list