JDK-8219774 is relevant to this patch (this was discussed in the code
review for JDK-8219378: NPE in ReflectionFactory.newMethodAccessor
when langReflectAccess not initialized).
This cleans up the initialization of LangReflectAccess:
http://cr.openjdk.java.net/~mchung/jdk14/8219774/webrev.00/
I moved LangReflectAccess to jdk.internal.access to be consistent with
other *Access classes (LangReflectAccess was added before the common
SharedSecrets class was introduced). AccessibleObject was initialized
early during startup and this patch causes initializing SharedSecret and
LangReflectAccess earlier. This is not an official review for JDK-8219774
but I'm interested if this improves the performance comparing with
Ogata's patch.
Ogato - can you help running the micro benchmark you have with
the patch for JDK-8219774?
thanks
Mandy
On 7/18/19 2:53 AM, Claes Redestad wrote:
Hi David,
On 2019-07-18 06:26, David Holmes wrote:
Hi Claes,
On 18/07/2019 1:04 am, Claes Redestad wrote:
Hi,
removing volatile aligns LangReflectAccess initialization with the
pattern used for other access objects: it's only initialized in the
static initializer of some class which we ensure is initialized, which
means any initialization race is guarded by the initialization of said
class - in this case j.l.r.Modifier.
If the field is not volatile then we are not guaranteed to correctly
see the constructed LangReflectAccess instance. Without volatile (or
without additional use of unconditional sync in the accessor) we do
not have a happens-before relationship between the construction of
the LRA instance, and the setting of the field >
Initialization of other access objects are not guarded by any
explicit synchronization, however, since similar load/store barriers
will be in effect by ensuring the class that constructs the object has
been initialized. So I think you could remove the explicit
synchronization.
We are not guaranteed to hit the class initialization checks that
would guarantee the necessary ordering.
You better not look at the existing code in
jdk.internal.access.SharedSecrets :-)
Would the unsafe.ensureClassInitialized(..) pattern used there bring
stronger guarantees? That would be the pattern I'm suggesting the code
under review here to align with.
And for the record none of the *Access objects carry state and are
strictly delegating to either static methods/constructors or instance
methods on their arguments.
/Claes
David
-----
I'm not sure why LangReflectAccess has not moved in with other
SharedSecrets. Perhaps this is just an artefact of having been around
for a long time, but maybe that'd cause some cyclic bootstrap
dependency. Either way it's nice to see it align to use the same
pattern.
Thanks!
/Claes
On 2019-07-17 10:00, Kazunori Ogata wrote:
Hi Aleskey,
Thank you for your comment.
I forgot to mention one thing. I verified that all accesses to
langReflectioAccess calls the accessor "langReflectAccess()".
Since this
variable is modified once from null to non-null, I think the write
in the
setter is guaranteed to be visible in the getter by putting the
synchronized block in langReflectAccess().
Should I put comments about this assumption? Actually, in JDK11
there are
some lines that do not call the getter, so backport needs to fix them,
too.
Regards,
Ogata
Aleksey Shipilev <sh...@redhat.com> wrote on 2019/07/17 16:49:08:
From: Aleksey Shipilev <sh...@redhat.com>
To: Kazunori Ogata <oga...@jp.ibm.com>,
core-libs-dev@openjdk.java.net
Date: 2019/07/17 16:49
Subject: [EXTERNAL] Re: RFR: JDK-8227831: Avoid using volatile for
write-
once, read-many class field
On 7/17/19 8:48 AM, Kazunori Ogata wrote:
Webrev: http://cr.openjdk.java.net/~ogatak/8227831/webrev/
Note this is generally not safe: it introduces data race on
langReflectAccess field access. It has
to be proved that everything that implements LangReflectAccess
interface
makes this data race
benign: e.g. all fields that are accessed via those implementation
are
final, read once, etc. And
briefly looking at that, I am not sure it is the case for the actual
accessor generators.
I'd cautiously leave "volatile" here.
--
Thanks,
-Aleksey
[attachment "signature.asc" deleted by Kazunori Ogata/Japan/IBM]
