Hi Mandy, Thank you for your reply. I measured your patch, and the performance was about 4% better than mine. Since it is faster and cleaner, I agree your patch looks better.
Regards, Ogata "core-libs-dev" <core-libs-dev-boun...@openjdk.java.net> wrote on 2019/07/19 02:58:50: > From: Mandy Chung <mandy.ch...@oracle.com> > To: Claes Redestad <claes.redes...@oracle.com>, David Holmes > <david.hol...@oracle.com> > Cc: core-libs-dev@openjdk.java.net > Date: 2019/07/19 02:59 > Subject: [EXTERNAL] Re: RFR: JDK-8227831: Avoid using volatile for write- > once, read-many class field > Sent by: "core-libs-dev" <core-libs-dev-boun...@openjdk.java.net> > > 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] > >>>> > >
