[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: 8229258: Rework markOop and markOopDesc into a simpler mark word value carrier
From: Kim Barrett <kim.barrett () oracle ! com>
Date: 2019-08-16 18:33:54
Message-ID: 5C08780B-5DB3-4E6E-A4A4-3257BF90E39B () oracle ! com
[Download RAW message or body]
> On Aug 16, 2019, at 4:20 AM, Stefan Karlsson <stefan.karlsson@oracle.com> wrote:
>
> On 2019-08-16 00:59, Kim Barrett wrote:
> > src/hotspot/share/oops/markOop.hpp
> > 109 template<typename T> operator T();
> > My mistake in the earlier review comment. Function should be const
> > qualified, e.g. that should be
> > template<typename T> operator T() const;
>
> I added this after one of our earlier discussions. However, I don't think we need \
> it (const or not). We already get sensible compiler errors without this function \
> when we try to cast markWords to something else:
> void* p0 = m;
> void* p1 = (void*)m;
> int i0 = m;
> int i1 = (int)m;
>
> [… various errors …]
You're right. It seems I need to refresh my recollection of what the valid \
conversions are.
> The poisoned constructor seems to be unnecessary as well, now that we have \
> simplified markWord. Without it, I get appropriate error messages when I try to \
> create a markWord from a pointer:
> error: invalid conversion from ‘void*' to ‘uintptr_t' {aka ‘long unsigned \
> int'} [-fpermissive] markWord p((void*)0x111);
> ^~~~~~~~~~~~
> note: initializing argument 1 of ‘markWord::markWord(uintptr_t)'
> explicit markWord(uintptr_t value) : _value(value) { }
I no longer recall why that one was even suggested. But you are right that it isn't \
needed.
> I've removed both of these.
Good.
> […]
>
> I'll leave the rest of the comments below for follow-up RFEs.
OK.
> This is the last few cleanups:
> http://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.04.delta/
> http://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.04/
Looks good.
> I ran extended testing on .03 (tier1-7 on linux), checked the markWord functions \
> were inlined, and checked that the generated code for \
> G1ParScanThreadState::copy_to_survivor_space was the same before and after the \
> patch. So I intend to run tier1 testing on .04 and then push this patch.
Thanks for checking the generated code. It's what we expected, but compilers are \
sometimes surprising.
[prev in list] [next in list] [prev in thread] [next in thread]