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

[Stefan Karlsson]

Thanks Martin!

StefanK

On 2019-08-15 17:07, Doerr, Martin wrote:
> Hi Stefan,
>
>> 1) Platform maintainers probably want to run this patch through their
>> build system.
> builds on PPC64 and s390.
>
> Best regards,
> Martin
>
>
>> -----Original Message-----
>> From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf Of
>> coleen.phillimore at oracle.com
>> Sent: Donnerstag, 15. August 2019 14:57
>> To: hotspot-dev at openjdk.java.net
>> Subject: Re: RFR: 8229258: Rework markOop and markOopDesc into a simpler
>> mark word value carrier
>>
>>
>> https://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.03.d
>> elta/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));
>>
>>
>> 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.
>>
>> This is a massive improvement.  Dont' change a thing and check it in.
>> We can do minor cleanups in followup RFEs.
>>
>> 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.d
>> elta/
>>>
>> 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
>>>>