RFR: 8229258: Make markOopDesc AllStatic

[Stefan Karlsson]

On 2019-08-08 14:18, coleen.phillimore at oracle.com wrote:
> 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.

Hmm. You are right. I generated an incorrect webrev. I reran webrev with 
-r qparent and now have this:
http://cr.openjdk.java.net/~stefank/8229258/webrev.02/

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

I got the same feedback from Kim but I'm personally not keen on making 
the name any longer. Specially now when MarkWord:: is being appended to 
all mark word operations.

If enough reviewers push back on this, then I guess I have to change this.

Thanks,
StefanK


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