RFR (XS): 8220345: Use appropriate type for G1RemSetScanState::IsDirtyRegionState
Thomas Schatzl
thomas.schatzl at oracle.com
Fri Mar 8 20:37:24 UTC 2019
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
More information about the hotspot-gc-dev
mailing list