RFR: 8229258: Make markOopDesc AllStatic

[Kim Barrett]

> On Aug 12, 2019, at 12:19 PM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
> Hi Roman,
> 
> Kim helped me figuring out how to get past the volatile issues I had with the class markWord { uintptr_t value; ... } version. So, I've created a version with that:
> 
> https://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.01/
> 
> I can go with either approach, so let me now what you all think.
> 
> One drawback of this approach is all the places I had to add .value() calls to get the raw bits. That's mostly done in the compilers and in logging code.

Well, that was a bit of a slog.  I've reviewed all of the C++ changes.
I haven't looked at the Java changes at all.  Let me know if you need
me to do so.

I like the approach. I think there are some remaining issues, but I
think this mostly looks good.  First a few broader issues:

(1) I found one area of code where I thought there might be some
significant cleanup needed, but then realized it was all CMS-specific,
so I'm not going to go into that. Better to just do the minimum for
now for this change, and delete that code soon.

(2) You mentioned the need for .value() calls as a drawback. I think
it's actually overall beneficial. I kept rough track while reviewing,
and I think the majority (by about 2:1) of those calls have their
result cast to intptr_t (or intx, or long(!) in 64bit-specific code).
I think most/all of those casts could be removed. In the baseline code
we instead had reinterpret_casts of a pointer to an intptr_t.

Some of the remaining calls to .value() are then subject to a cast or
conversion to some pointer type. Perhaps a "to_pointer" operation
should be provided for these? (And maybe prevent non-use of to_pointer
by poisoning template<T> operator T*().)

(3) I think all explicit type conversions to markWord (e.g. using a
C-style cast to markWord) should be eliminated. In the old code we
have three cases:

(a) Many were reinterpret_casts of an integral value to markOopDesc*.

(b) Some of the remaining were pointer to markOopDesc* casts; either
static (down) casts where the value is void* or pointer to oopDesc* or
a derivative.

(c) Remainder were reinterpret_cast for other pointer conversions
conversions.

With markWord (a) are now conversions using the constructor, and are
better written using constructor syntax.  The pointer to markWord
conversions should be using markWord::from_pointer.  (We might enforce
that by poisoning markWord(const volatile void*).)

(4) I didn't bother to note misaligned comments and such; you've
already said you'll deal with these later, assuming this is the
approach taken.

And now a few more specific issues:

------------------------------------------------------------------------------
[pre-existing, optional change]

assert in all MacroAssembler::biased_locking_enter
  assert(markWord::age_shift == markWord::lock_bits + markWord::biased_lock_bits,
         "biased locking makes assumptions about bit layout");

I think these can all be STATIC_ASSERTs.

------------------------------------------------------------------------------
src/hotspot/share/oops/markOop.hpp
 341   markWord clear_lock_bits() { return markWord(value() & ~lock_mask_in_place); }

I think this should be returning some kind of pointer, and not a
markWord.  That would also remove the need for the cast to void* of
the .value() in MarkWord::decode_pointer().

The kind of pointer for the result should be oopDesc* ?

Also removes the need for the reinterpret_cast here:
src/hotspot/share/gc/shenandoah/shenandoahForwarding.inline.hpp
  58     return oop(reinterpret_cast<oopDesc*>(old_mark->clear_lock_bits()));
  66     return oop(reinterpret_cast<oopDesc*>(prev_mark->clear_lock_bits()));

------------------------------------------------------------------------------
[pre-existing, not your problem, but I can't help but question this.]

src/hotspot/share/jfr/leakprofiler/utilities/saveRestore.cpp
  51 MarkWordContext::MarkWordContext(const MarkWordContext& rhs) : _obj(NULL), _mark_word(0) {
  52   swap(const_cast<MarkWordContext&>(rhs));
  53 }

A copy constructor that modifies the source for the copy?  That seems
very weird and confusing.

------------------------------------------------------------------------------
src/hotspot/share/memory/heap.cpp
 418 d

Stray character.

------------------------------------------------------------------------------
src/hotspot/share/oops/oop.cpp

Comment needs some updating:
 108   // Header verification: the mark is typically non-NULL. If we're
 109   // at a safepoint, it must not be null.

------------------------------------------------------------------------------
src/hotspot/share/oops/oop.inline.hpp.
  46 markWord oopDesc::mark() const {
  47   uintptr_t v = HeapAccess<MO_VOLATILE>::load_at(as_oop(), mark_offset_in_bytes());
  48   return markWord(v);
  49 }

I was wondering why this wasn't just:

markWord oopDesc::mark() const {
  return HeapAccess<MO_VOLATILE>::load_at(as_oop(), mark_offset_in_bytes());
}

But that would hit the (intentionally) undefined copy-from-volatile
constructor for markWord.

I think it would work if the Access API (in particular, LoadAtProxy)
paid attention to PrimitiveConversion::Translate handlers. But we
wouldn't want it to pay attention to the translators for oop and its
subclasses.

So without more careful thought than I want to apply right now, I
think the webrev definition is the way to go.

And if Access loads paid attention to translate handlers, probably
stores should too, which would remove the need for the value() call
here:

  71 void oopDesc::release_set_mark(markWord m) {
  72   HeapAccess<MO_RELEASE>::store_at(as_oop(), mark_offset_in_bytes(), m.value());
  73 }

Something to think about for the future?

------------------------------------------------------------------------------
src/hotspot/share/runtime/basicLock.cpp
  31   markWord moop = displaced_header();

Variable name should be updated.

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