RFR: 8229258: Make markOopDesc AllStatic

[Stefan Karlsson]

On 2019-08-08 12:22, Roman Kennke wrote:
> First glance.
> 
> Instead of making it AllStatic, I'd probably have made it a plain object:
> 
> class markWord {
> }
> 
> and instead of changing e.g.:
> 
> obj->mark().set_unlocked();
> 
>   to
> 
> MarkWord::set_unlocked(obj->mark());
> 
> leave it like it is. Looks/feels better to me.
> 
> Have you tried that? Is this too tricky to do? Or what is the reason to
> use static accessors there?

Not sure if you mean that we should still play tricks with the this 
pointer. If that's the case then, that would retain the undefined 
behavior that this patch removes (by using the static accessors). One 
example given below is:

bool is_being_inflated() const { return (value() == 0); }

which ends up comparing this to NULL.

But maybe you suggests adding the mark word bits as a value in MarkWord? 
Something like this:

class MarkWord {
   uintptr_t _value;

   // and keep the functions as non-static functions
   bool is_being_inflated() const { return (_value == 0); }
}	

then, yes, I tried that because I also felt that it was unfortunate that 
I had to retransform the code in this way. However, I backed off because 
of different hurdles that I couldn't trivially solve. For example:

class oopDesc {
   volatile markWord _mark;
}
...
_mark = prototype(); <-- complains here that the assignment operator 
discards volatile qualifier

I'm unsure what performance effects it will have if I provide a 
hand-written assignment operator, since the class will not be a trivial 
class anymore. Because of that, I backed away from such a change.

I'm not at all opposed to wait with this patch if someone wants to take 
the time to retry that experiment.

Thanks,
StefanK

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