RFR: 8229258: Make markOopDesc AllStatic

[Stefan Karlsson]

Hi Roman,

Kim helped me figuring out how to get past the volatile issues I had 
with the class markWord { uintptr_t value; ... } version. So, I've 
created a version with that:

https://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.01/

I can go with either approach, so let me now what you all think.

One drawback of this approach is all the places I had to add .value() 
calls to get the raw bits. That's mostly done in the compilers and in 
logging code.

Inlined:

On 2019-08-09 00:57, Roman Kennke wrote:
> Hi Stefan,
>
> Awesome work!

Thanks. :)

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

Thanks.

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

I agree. When we've decided what approach to take, we might want to 
explicitly ping them.

>
> - 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 haven't done that yet, but will do it.

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

Would be good to get someone to review those parts ...

Thanks for looking at this,
StefanK
>
>
> 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
>>