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

[Kim Barrett]

> On Jun 14, 2020, at 4:51 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
> 
>> On Jun 10, 2020, at 8:46 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
>> 
>> […]
>> 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
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/atomic.hpp 
> 1011 template<typename T>
> 1012 static AtomicValue<T>* cast_to_atomic_ptr(T* nonatomic) {
> 
> I would really like to not have this.  I'd like to see how far we can
> get without it.
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/atomic.hpp 
> 940 class AtomicValue {
> 
> Should it be default constructible? If so, should the default
> constructor guarantee value initialization? I think having a trivial
> default constructor is needed to avoid UB in some uses. See discussion
> of G1BOT's _offset_array below.
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1BlockOffsetTable.cpp
> 45   _offset_array = cast_to_atomic_ptr((u_char*) bot_reserved.start());
> 
> This is currently the only use of cast_to_atomic_ptr. It leads to UB
> since the (non-trivial) constructor for AtomicValue is never called
> for any of the objects in the array. I think the call to
> memset_with_concurrent_readers is also UB. I *think* having a trivial
> default constructor solves the UB problem, but that isn't possible
> until C++11.
> 
> Rather than just casting to a pointer to AtomicValue<>, this could
> iterate over the array memory, doing placement new construction of the
> elements.  If we gave AtomicValue<> a non-trivial but empty default
> constructor (so not initializing the value), then such an iteration
> should be optimized away.  We'd still have the UB from memset_with_cr,
> though that could use an iteration with the one-arg constructor instead.
> Or ignore the technical UB and just keep using memset_with_cr.
> 
> And that gets rid of the one use (so far) of cast_to_atomic_ptr.
> 
> The problem, of course, is that having a trivial/empty default
> constructor means one could unintentionally construct without
> initialization. I think a private trivial default constructor would
> deal with that, though again that requires C++11.

Responding to myself about the above, here’s a way to get value
initialization by the default constructor but have another constructor that
does no initialization, and should be compiled to nothing.

// Put this somewhere convenient (not! in AtomicValue class).
struct NoInitFlag {};
const NoInitFlag NO_INIT;

Add these constructors:
AtomicValue() : _val() {}   // Value initialization.
AtomicValue(NoInitFlag) {}  // Intentionally implicit.

> src/hotspot/share/gc/g1/heapRegionManager.cpp
> 663   for (uint i = 0; i < _n_regions; ++i) {
> 664     new (&_claims[i]) AtomicValue<uint>(Unclaimed);
> 665   }
> 
> Even if AtomicValue has a trivial default destructor (as discussed
> above) to avoid UB when using memset, this change is an improvement.
> The old memset code only works because Unclaimed == 0.

Also related, s/destructor/constructor/