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

[Thomas Schatzl]

Hi Kim,

On Fri, 2019-03-08 at 14:59 -0500, Kim Barrett wrote:
> > 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
> > ?
> > 
[...]
> -------------------------------------------------------------------
> -----------
> 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.
> 
> -------------------------------------------------------------------
> ---

All fixed.

http://cr.openjdk.java.net/~tschatzl/8220345/webrev.0_to_1/ (diff)
http://cr.openjdk.java.net/~tschatzl/8220345/webrev.1/ (full)

Thanks,
  Thomas