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.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 >> >
signature.asc
Description: OpenPGP digital signature