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

[Roman Kennke]

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.

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

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?

I say ship it!

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
>>
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190815/a59c8c0b/signature.asc>