RFR: 8229258: Make markOopDesc AllStatic

[Roman Kennke]

Hi Stefan,

Awesome work!

I have a few comments:

- You're going to need this patch on top of yours to keep Shenandoah in
shape: :-)
http://cr.openjdk.java.net/~rkennke/8229258-shenandoah/webrev.00/

- Would be good to get as many affected platforms tested as possible.
There are changes like the ones in Shenandoah above that are not easy to
catch by grepping. Maybe reach out to folks who are known to regularily
build fancy platforms like SAP, Bellsoft,.. ?

- There are a few places where alignment of comments get messed up, e.g.:

src/hotspot/cpu/ppc/macroAssembler_ppc.cpp:

   ld(mark_word, oopDesc::mark_offset_in_bytes(), obj);         //
Reload in transaction, conflicts need to be tracked.
-  andi(R0, mark_word, markOopDesc::biased_lock_mask_in_place); // look
at 3 lock bits
-  cmpwi(flag, R0, markOopDesc::unlocked_value);                // bits
= 001 unlocked
+  andi(R0, mark_word, MarkWord::biased_lock_mask_in_place); // look at
3 lock bits
+  cmpwi(flag, R0, MarkWord::unlocked_value);                // bits =
001 unlocked
   beq(flag, DONE_LABEL);                                       // all
done if unlocked

I haven't collected all those places (if any more), but would be good if
you could sweep over the patch and fix those.

- I agree with Coleen that markWord vs. MarkWord is a bit confusing. Not
sure what to do though.

- I know it would probably disturb even more code, but shouldn't the
files markOop.* be renamed to markWord.* ?

- The changes concerning SA, Graal and JRF *look* ok to me, but I don't
feel qualified to ack them.


Thanks,
Roman


> Hi all,
> 
> Please review this patch to make markOopDesc AllStatic.
> 
> https://cr.openjdk.java.net/~stefank/8229258/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-8229258
> 
> From the RFE:
> 
> I want to change/rename markOop and markOopDesc
> 
> Today markOopDescs inherits from oopDesc even though they are not
> oopDescs (Java objects):
>  `class markOopDesc : public oopDesc {`
> 
> This is very confusing to new-comers (and probably to old-timers as well).
> 
> A simple fix would be to break the inheritance and do the following rename:
> markOopDesc -> MarkWord
> markOop -> markWord
> 
> However, there are more dubious code in this area. markOopDescs are
> created and used like this:
> 
> ```
> class oopDesc {
> ...
>   volatile markOop _mark;
> ...
> markOop  oopDesc::mark_raw()  const { return _mark; }
> ...
> // Usage
> obj->mark_raw()->is_marked()
> ...
> // Implementation
>   bool is_marked()   const {
>     return (mask_bits(value(), lock_mask_in_place) == marked_value);
>   }
> ...
> // Implementation
>   bool is_being_inflated() const { return (value() == 0); }
> ...
> // Implementation of markOopDesc::value()
>   uintptr_t value() const { return (uintptr_t) this; }
> ```
> 
> Remember, _mark is an arbitrary bit pattern describing the object. We
> treat it as if it were a pointer to a markOopDesc object, but it's not
> pointing to such an object. While using that (weird) pointer we call
> value() and extract it's bit pattern to be used for further bit pattern
> checking functions. AFAICT, this is also undefined behavior. At least
> is_being_inflated() const is effectively 'return (uintptr_t) this ==
> NULL'. This UB was recently discussed here:
>  https://mail.openjdk.java.net/pipermail/hotspot-dev/2019-July/038704.html
>  https://mail.openjdk.java.net/pipermail/hotspot-dev/2019-July/038712.html
> 
> I propose that we change MarkWord (markOopDesc) to be an AllStatic class
> instead and get rid of this (ab)use of the markWord (markOop) pointer.
> 
> The patch is large but mostly straight forward. However, there is some
> Java code that would need some extra scrutiny: SA and JVMCI.
> 
> Tested with tier1-3.
> 
> Coleen has asked for file rename from markOop.hpp to markWord.hpp. I'd
> like to do that as a separate RFE when/if this patch gets accepted.
> 
> Thanks,
> StefanK
>