RFR (XS): 8220345: Use appropriate type for G1RemSetScanState::IsDirtyRegionState

[Kim Barrett]

> On Mar 8, 2019, at 8:51 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi all,
> 
>  can I have reviews for this small change that removes the use of
> jbyte for an internal data type in favor of the initially intended bool
> ?
> 
> The original reason to do so was "G1RemSetScanState::IsDirtyRegionState
> is a jbyte because "since there are no atomic instructions for
> bools".", but this is obviously no longer true, and the workaround can
> be removed.
> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8220345
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8220345/webrev/
> Testing:
> hs-tier1-5
> 
> Thanks,
>  Thomas

The size of bool is implementation defined (see C++ 5.3.3).  Our
atomics support doesn't cover all sizes, though I think we're okay
here for currently supported platforms.

OTOH, while there is old code that is "careful" about this, and I was
being careful about it, I've noticed there are a number of more recent
places using atomic bool.  (I may be guilty of some myself now.)

So I'm not sure how much importance to attach to the potential
problem this change is introducing.  It's far from the first, so maybe
that ship has sailed.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1RemSet.cpp
Rather than

 245     bool marked_as_dirty = Atomic::cmpxchg(true, &_in_dirty_region_buffer[region], false) == false;
 246     if (marked_as_dirty) {

I would prefer

 245     bool marked_as_dirty = !Atomic::cmpxchg(true, &_in_dirty_region_buffer[region], false);
 246     if (marked_as_dirty) {

or even

 246     if (!Atomic::cmpxchg(true, &_in_dirty_region_buffer[region], false)) {

------------------------------------------------------------------------------ 
src/hotspot/share/gc/g1/g1RemSet.cpp
 119   bool* _in_dirty_region_buffer;

Shouldn't the type be "volatile bool*" ?  We've been using volatile
qualifiers as a marker for values manipulated by atomics.

Of course, that will affect the memset for clearing the array.

------------------------------------------------------------------------------