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

[Stefan Karlsson]

On 2019-08-15 14:56, coleen.phillimore at oracle.com wrote:
>
> https://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.03.delta/src/hotspot/share/runtime/synchronizer.cpp.udiff.html 
>
>
> + lock->set_displaced_header(markWord::from_pointer(NULL));
>
>
> Should this be markWork::zero ?
>
> + m->set_header(markWord(markWord::zero));

I can change it to that if you prefer it like that.

>
>
> ew.
>
> I just clicked through the latest incremental and the full oops 
> directory.
>
> The to_pointer and from_pointer casts are awkward, particularly 
> because you can do (Edge*)mark.value() too, but it's good to find 
> where we're using the markWord for various purposes.

I think Kim preferred the to_pointer() calls, and I tried to stay away 
from using .value() as much as possible. Using the value() function is a 
way to break free from the markWord abstraction and do arbitrary bit 
manipulations on the mark word, instead of going through the markWord 
API. I'd prefer if we used value() as little as possible. Regarding the 
example above, we could also change the code to this: 
mark.to_pointer<Edge*>().

>
> This is a massive improvement.  Dont' change a thing and check it in.  
> We can do minor cleanups in followup RFEs.

Yes. I think this is the most sane way to proceed with this.

Thanks for reviewing!

StefanK

>
> Thanks!
> Coleen
>
>
> On 8/15/19 7:46 AM, Stefan Karlsson 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:
>>
>> 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
>>>
>>
>