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

[Stefan Karlsson]

Hi Roman,

On 2019-08-15 19:06, Roman Kennke wrote:
> Hi Stefan,
>
> I looked over the changes again. I like this much better, a huge
> improvement over current state, and also better than your first
> proposal. I also prefer the explicit value() calls.

Great!

>
> I also built+tested Shenandoah GC again, seems all fine.
>
> Didn't know that C++ has an 'explicit' specifier. Oh man.
>
> Still seems to have foobared alignment (it was partly kaputted before
> already):
> src/hotspot/share/oops/oopsHierarchy.hpp

You're right. I removed one stray whitespace:
$ hg diff
diff --git a/src/hotspot/share/oops/oopsHierarchy.hpp 
b/src/hotspot/share/oops/oopsHierarchy.hpp
--- a/src/hotspot/share/oops/oopsHierarchy.hpp
+++ b/src/hotspot/share/oops/oopsHierarchy.hpp
@@ -46,7 +46,7 @@
  typedef class   instanceOopDesc*            instanceOop;
  typedef class   arrayOopDesc*               arrayOop;
  typedef class     objArrayOopDesc*            objArrayOop;
-typedef class     typeArrayOopDesc*            typeArrayOop;
+typedef class     typeArrayOopDesc*           typeArrayOop;

  #else

I think the other indentation is done on purpose:
typedef class oopDesc*                    oop;
typedef class   instanceOopDesc*            instanceOop;
typedef class   arrayOopDesc*               arrayOop;
typedef class     objArrayOopDesc*            objArrayOop;
typedef class     typeArrayOopDesc*           typeArrayOop;

to show the oops hierarchy.

>
> Out of curiosity, what's with the changes in objectMonitor.inline.hpp to
> access the markWord atomically?:
>
> -inline markOop ObjectMonitor::header() const {
> -  return _header;
> +inline markWord ObjectMonitor::header() const {
> +  return Atomic::load(&_header);
>   }
>
> I guess this is good (equal or stronger than before) but is there a
> rationale behind these changes?

Ahh. Right. That was done to solve the problems I were having with 
volatiles. For example:
src/hotspot/share/runtime/objectMonitor.inline.hpp:38:10: error: binding 
reference of type 'const markWord&' to 'const volatile markWord' 
discards qualifiers
    return _header;

and:
src/hotspot/share/runtime/basicLock.hpp:40:74: error: implicit 
dereference will not access object of type ‘volatile markWord’ in 
statement [-Werror]
   void         set_displaced_header(markWord header) { 
_displaced_header = header; }

Kim suggested that the fact that these fields were volatile was an 
indication that we should be doing some kind of atomic/ordered 
operation. By replacing these loads and stores with calls to the Atomic 
APIs, and providing the PrimitiveConversions::Translate<markWord> 
specialization, we could solve that problem.

>
> I say ship it!

Thanks a lot for reviewing this!

StefanK

>
> Thanks,
> Roman
>
>
>> 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:
>>
>> 1) Platform maintainers probably want to run this patch through their
>> build system.
>> 2) SA maintainers (CC:ed serviceability-dev)
>> 3) JVMCI maintainers
>>
>> Thanks,
>> StefanK
>>
>> On 2019-08-14 11:11, Roman Kennke wrote:
>>> Am 14.08.19 um 01:26 schrieb 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.
>>>> I've finally had time to look at the first proposed change.
>>>>
>>>> Comparing the first approach (an AllStatic MarkWord class and markWord
>>>> typedef'd to uintptr_t) vs the second approach (markWord is a thin
>>>> class wrapping around uintptr_t), I prefer the second.
>>>>
>>>> * I think the markWord class provides better type safety. It still
>>>> involves too many casts sprinkled over the code base, but I think it
>>>> also provides a better basis for further cast reduction and
>>>> prevention.
>>>>
>>>> * I think having one markWord class for the data and behavior is
>>>> better / more natural than having a markWord typedef for the data and
>>>> a MarkWord AllStatic class for the behaviour.
>>>>
>>>> * I like that the markWord class eliminates the markWord vs MarkWord
>>>> homonyms, which I think will be annoying.
>>>>
>>>> * The markWord class is a trivially copyable class, allowing it to be
>>>> efficiently passed around by value, so no disadvantage there.
>>>>
>>>> I haven't found anything that I think argues for the first over the
>>>> second. Other folks might have different priorities or taste. I think
>>>> either is better than the status quo.
>>>>
>>>> I'm still reviewing webrev.valueMarkWord.02, but so far haven't found
>>>> anything that makes me want to suggest backing off from that direction.
>>>>
>>>> Note that the bug summary doesn't describe the second approach.
>>> +1 :-)
>>>
>>> Roman
>>>