RFR: 8229258: Make markOopDesc AllStatic

Thu Aug 8 20:16:50 UTC 2019

> http://cr.openjdk.java.net/~stefank/8229258/webrev.02/ 

I reviewed the following files via the Udiffs links:

   src/hotspot/share/oops/markOop.cpp
   src/hotspot/share/oops/markOop.hpp
   src/hotspot/share/oops/markOop.inline.hpp
   src/hotspot/share/runtime/basicLock.cpp
   src/hotspot/share/runtime/basicLock.hpp
   src/hotspot/share/runtime/biasedLocking.cpp
   src/hotspot/share/runtime/biasedLocking.hpp
   src/hotspot/share/runtime/objectMonitor.cpp
   src/hotspot/share/runtime/objectMonitor.hpp
   src/hotspot/share/runtime/objectMonitor.inline.hpp
   src/hotspot/share/runtime/synchronizer.cpp
   src/hotspot/share/runtime/thread.cpp
   src/hotspot/share/runtime/vframe.cpp

and the changes look fine. This is a massive amount of work, but
it is a necessary cleanup. Thanks for tackling it!

Thumbs up (on the above files)!

Dan

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