RFR (M): 8184346: Clean up G1CMBitmap

[Roman Kennke]

Hi Thomas,

> 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.
Fine for me.
>> 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.
Ok.

>>> 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.
That's fine for me. Just wanted to point out that this is going to come :-)
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8184346/webrev.1/ (full)
> http://cr.openjdk.java.net/~tschatzl/8184346/webrev.0_to_1/ (diff)

Good for me.

Roman