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

[Thomas Schatzl]

Hi,

On Mon, 2019-03-11 at 15:29 -0400, 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 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.

I haven't spotted anything either.

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

I did the latter. I do not see an advantage to repeat the same assert
basically everywhere. I added a note about what to look for to find the
places easily.

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

I fixed the visibility here and in several other locations.

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

Fixed.

> -------------------------------------------------------------------
> ----------- 
> src/hotspot/share/gc/g1/g1RemSet.[ch]hpp
> 
> Seems like there are enough CardTable::CardValue uses in this class
> to be worth a typedef.

Added.

http://cr.openjdk.java.net/~tschatzl/8220301/webrev.1_to_2/ (diff)
http://cr.openjdk.java.net/~tschatzl/8220301/webrev.2/ (full)

webrevs.

Thanks,
  Thomas