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

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 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.karls...@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

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to