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

[Kim Barrett]

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

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

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

------------------------------------------------------------------------------


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.

------------------------------------------------------------------------------

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

------------------------------------------------------------------------------