RFR (M): 8184346: Clean up G1CMBitmap

[Thomas Schatzl]

Hi Roman,

On Fri, 2017-07-14 at 11:53 +0200, Roman Kennke wrote:
> Hi Thomas,
> 
> > Hi all,
> > 
> >   can I have reviews for this change that tries to clean up (and
> > only clean up) the G1CMBitMap class (and the surrounding helper
> > classes) a bit?
> > 
> > What has been done:
> > - fix naming
> > - improve visibility of methods
> > - remove superfluous code
> > - make G1CMBitMapClosure pass a HeapWord* instead of a bitmap
> > index, avoiding that the user code is cluttered with conversions
> > from bitmap indices to HeapWords
> > - remove inheritance between G1CMBitMap and G1CMBitMapRO, similar
> > to the BitMap class make G1CMBitMapRO a "view" of G1CMBitMap.
> > - remove unused code in G1CMBitMapRO
> > - move method implementations into .inline.hpp file
>  The changes look good to me.

Thanks for your review.

> + return _cm->nextMarkBitMap()->is_marked((HeapWord *)obj);
> 
> I'd write that as
> 
> (HeapWord*) obj
> 
> but I'm never quite sure what style is preferable in Hotspot ;-)

I do not know either :) I would kind of prefer no space between cast
and the variable, as casts to me are something like unary operators
where we do not add a space between operator and variable either.

I removed the space between the type and the star at least.

> Are changes in  g1FromCardCache.cpp/.hpp unrelated?

Yes, sorry. I will remove those and send out an extra RFR. I forgot to
split them out.

> > The next CR JDK-8184347 will deal with moving G1CMBitmap* into
> > separate
> > files.
>  And while you're at it, you may want to move it to gc/shared and
> renamed it to something like MarkBitmap?
> https://bugs.openjdk.java.net/browse/JDK-8180193
> 

Not particularly against this change, but I think we should do the move
and renaming separately when the change is actually required, i.e. just
before there is another dependency on it.

Also, G1CMBitMap has hard dependencies on several other G1 specific
classes, so I think it is too early to move it to the shared directory
from that POV too.

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

Thanks,
  Thomas