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

[Thomas Schatzl]

Hi,

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?
> > 
> > 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.

I considered whether this is an issue, and I am aware of that bool is
implementation defined, but thought there wouldn't be any issue.

-------------------------------------------------------------------
> -----------
> 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)) {

I intentionally kept it this way to keep the existing pattern of
Atomic::cmpxchg uses; I was aware of the optimization but did not
really like it because of decreased readability. Apparently you do not
consider it important, I will change this.

> -------------------------------------------------------------------
> ----------- 
> 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.

Exactly that's the reason why I did not make it volatile. I will change
that as well.

Thanks,
  Thomas