RFR (M): 8220301: Remove jbyte use in CardTable (affects all platforms)

[Kim Barrett]

> On Mar 8, 2019, at 8:51 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi all,
> 
> can I have reviews for this change the replaces the use of the JNI
> data type jbyte in the CardTable class by uint8_t via the custom
> CardValue typedef?
> 
> This cleanup stems from trying to get rid of JNI data types in VM/GC
> code; formerly the main reason to use them here has been that the
> Atomic operations were not available for e.g. uint8_t, but that has
> been fixed for a long time.
> 
> While the change itself is a no-op in practice, and testing showed so,
> I would like non-Oracle platform maintainers to try to compile the
> change and report problems because of lack of build platforms.
> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8220301
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8220301/webrev/
> Testing:
> hs-tier1-5
> 
> Thanks,
>  Thomas

I'm concerned about changing the signed-ness of card table entries.
jbyte is signed.  There may be code that expects or assumes signed
values.  One place of particular concern would be assembly code
generators, where we might be generating sign-extending loads.

Note that I didn't actually spot any signed-ness problems, but they
would be easy to miss.

------------------------------------------------------------------------------ 
src/hotspot/cpu/aarch64/gc/g1/g1BarrierSetAssembler_aarch64.cpp
  44 STATIC_ASSERT(sizeof(CardTable::CardValue) == 1);

I think it is better to have assertions (static or otherwise) near the
code that depends on them for correctness.  The old asserts looked
like they were in the right places, this replacement STATIC_ASSERT is
not.

Similarly in lots of other places.

And there are two top-level near the beggining of the file static
asserts here:
src/hotspot/cpu/arm/gc/g1/g1BarrierSetAssembler_arm.cpp

An alternative would be to document and assert it once at the place
where CardValue is defined, and just say there's lots of code making
that assumption, so change at your peril.  That would also address
Aleksey's complaint.

------------------------------------------------------------------------------
src/hotspot/share/gc/cms/cmsCardTable.cpp
 230     CardValue* last_card_to_check =
 231       (CardValue*) MIN2((intptr_t) last_card_of_cur_chunk,
 232                         (intptr_t) last_card_of_first_obj);

pre-existing: Why all the casts here?  This would work fine with all
the casts removed.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1DirtyCardQueue.hpp
  40 protected:
  41   typedef CardTable::CardValue CardValue;

Not sure why this is being made protected rather than public.  Public
seems more natural, since it's the declared type of a parameter of a
public function.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1HotCardCache.cpp
  80   CardValue* previous_ptr = Atomic::cmpxchg(card_ptr,
  81                                                        &_hot_cache[masked_index],
  82                                                        current_ptr);

Argument indentation seems to have gotten messed up.

------------------------------------------------------------------------------ 
src/hotspot/share/gc/g1/g1RemSet.[ch]hpp

Seems like there are enough CardTable::CardValue uses in this class to
be worth a typedef.

------------------------------------------------------------------------------