RFR: 8229258: Make markOopDesc AllStatic

[coleen.phillimore at oracle.com]

Is this patch based off another in your queue?  It looks like MarkWord 
has already been introduced.

https://cr.openjdk.java.net/~stefank/8229258/webrev.01/src/hotspot/cpu/aarch64/aarch64.ad.udiff.html 


- // Set tmp to be (markOop of object | UNLOCK_VALUE).
+ // Set tmp to be (markWord of object | UNLOCK_VALUE).
      __ orr(tmp, disp_hdr, MarkWord::unlocked_value);
  

I was trying to see how many lines were changed to static function calls 
on MarkWord, vs obj->mark()->something().

My thought was it was not enough to try to make MarkWord contain 
markWord to enable this simpler syntax.  There are 543 MarkWord:: in 
your patch but some are constant references, which would still be there 
if you made MarkWord contain markWord.

I do have one request to think about.  Could MarkWord be a longer name 
like MarkWord{Impl}?  Don't like "Impl" though.  I don't want to 
bikeshed but it took me a while to spot that the 'm's were different in 
markWord and MarkWord.  Maybe that's the point but there's going to be a 
lot of mistyping.

Coleen

On 8/8/19 7:01 AM, Stefan Karlsson wrote:
>
>
> 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
>>>
>>