RFR: 8229258: Rework markOop and markOopDesc into a simpler mark word value carrier

Fri Aug 16 08:20:40 UTC 2019

On 2019-08-16 00:59, Kim Barrett wrote:
>> On Aug 15, 2019, at 7:46 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>>
>> Thanks Kim, Roman, Dan and Coleen for reviews and feedback.
>>
>> I rebased the patch, fixed more alignments, renamed the bug, and rerun the test through tier1-3.
>>
>> https://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.03.delta/
>> https://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.03/
>>
>> Could I get reviews for this version? I'd also like to ask others to at least partially look at this:
> 
> Here's my comments through webrev.valueMarkWord.03.
> 
> Looks good.  There are a couple of small fixes for which I don't need
> a new webrev, which are listed first below.  Then there are some
> broader items which could be addressed in followup improvements.
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/oops/markOop.hpp
>   109   template<typename T> operator T();
> 
> My mistake in the earlier review comment.  Function should be const
> qualified, e.g. that should be
> 
>    template<typename T> operator T() const;
> 

I added this after one of our earlier discussions. However, I don't 
think we need it (const or not). We already get sensible compiler errors 
without this function when we try to cast markWords to something else:

   void* p0 = m; 

   void* p1 = (void*)m; 

   int   i0 = m; 

   int   i1 = (int)m;

error: cannot convert ‘markWord’ to ‘void*’ in initialization 

    void* p0 = m; 

               ^ 

error: invalid cast from type ‘markWord’ to type ‘void*’ 

    void* p1 = (void*)m; 

                      ^ 

error: cannot convert ‘markWord’ to ‘int’ in initialization 

    int   i0 = m; 

               ^ 

error: invalid cast from type ‘markWord’ to type ‘int’ 

    int   i1 = (int)m; 

The poisoned constructor seems to be unnecessary as well, now that we 
have simplified markWord. Without it, I get appropriate error messages 
when I try to create a markWord from a pointer:

error: invalid conversion from ‘void*’ to ‘uintptr_t’ {aka ‘long 
unsigned int’} [-fpermissive] 

    markWord p((void*)0x111); 

               ^~~~~~~~~~~~ 

note:   initializing argument 1 of ‘markWord::markWord(uintptr_t)’ 

    explicit markWord(uintptr_t value) : _value(value) { }

I've removed both of these.

> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/biasedLocking.cpp
>   695                                              prototype.bias_epoch() == mark.bias_epoch())) {
> 
> I think one more leading space needs to be deleted to get proper
> alignment here.  Or reformat this long and complex if control
> expression.
> 

OK. I followed the pre-existing alignment, but I can change it anyway.

> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/vframe.cpp
>   244                         // FIXME: mark is set but not used below.
>   245                         //        Either the comment or the code is broken.
> 
> Is there a bug for this?
> 

Created JDK-8229808.

> ------------------------------------------------------------------------------
> 
> 
> The remainder seem like they could be followup improvements.
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/oops/markOop.hpp
>   138   static const uintptr_t zero     = 0;
> 
> All occurrences of this are either
> (1) markWord member initializater
> (2) markWord variable initializer
> (3) temporary markWord initializer
> 
> There don't appear to be any bare uses otherwise.  I think nicer is
> 
> static markWord zero() { return markWord(0); }
> 
> (For C++11: `static constexpr markWord zero = markWord(0);`)
> 
> This seems like it could be a followup improvement.
> 

I had the same thought and then backed away from it, but I can't 
remember why. This is a small enough change, so I've gone through the 
few occurrences and cleaned it up.

I'll leave the rest of the comments below for follow-up RFEs.

This is the last few cleanups:
http://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.04.delta/
http://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.04/

I ran extended testing on .03 (tier1-7 on linux), checked the markWord 
functions were inlined, and checked that the generated code for 
G1ParScanThreadState::copy_to_survivor_space was the same before and 
after the patch. So I intend to run tier1 testing on .04 and then push 
this patch.

Thanks,
StefanK

> ------------------------------------------------------------------------------
> 
> [This is also related to Coleen's comments about to_pointer.]
> 
> Looking at the changes, in order to reduce the amount of casting pixie
> dust sprinkled on our code base, I now think markWord::to_pointer
> should have a template overload, with the template parameter
> designating the return type.  And I think the return type for the
> non-template should be const qualified, and the template should handle
> cv qualifiers.  Like so (I think, I haven't actually tested this)
> 
>    const void* to_pointer() const {
>      return reinterpret_cast<void*>(_value);
>    }
> 
>    template<typename T>
>    T* to_pointer() const {
>      typedef typename RemoveCV<T>::type TT;
>      return reinterpret_cast<TT*>(_value);
>    }
> 
> If one wants a void* then use m.to_pointer<void>().
> 
> (I almost want to call it "pointer_to" so it reads "pointer to T".)
> 
> Coleen and I talked about a possible alternative to the template
> overload.  Perhaps there is a small and fixed number of pointer types
> we want to support conversion to, in which case we could have a small
> number of type-specific to_xxx_pointer variants?  But I'm not sure the
> number is actually small and fixed.
> 
> This seems like it could be a followup improvement.
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/interpreter/bytecodeInterpreter.cpp
> 
> In the run() function, there are a lot of casts of markWord::value()
> results or constants from markWord that I think could be removed.
> Some of them might want C++11 explicitly typed enums though.
> 
> This seems like it could be a followup improvement.
> 
> ------------------------------------------------------------------------------
> 