RFR: 8229258: Make markOopDesc AllStatic

Tue Aug 13 10:42:02 UTC 2019

Hi Kim,

I've addressed some of you comments and did an initial pass over the 
patch to clean up some alignment issues:

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

Thanks,
StefanK

On 2019-08-13 04:37, Kim Barrett wrote:
>> 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.
> 
> ------------------------------------------------------------------------------
> 