RFR: JDK-8227831: Avoid using volatile for write-once, read-many class field

[Kazunori Ogata]

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-bounces at openjdk.java.net> wrote on 
2019/07/19 02:58:50:

> From: Mandy Chung <mandy.chung at oracle.com>
> To: Claes Redestad <claes.redestad at oracle.com>, David Holmes 
> <david.holmes at oracle.com>
> Cc: core-libs-dev at 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-bounces at 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 <shade at redhat.com> wrote on 2019/07/17 16:49:08:
> >>>>
> >>>>> From: Aleksey Shipilev <shade at redhat.com>
> >>>>> To: Kazunori Ogata <OGATAK at jp.ibm.com>, 
> >>>>> core-libs-dev at 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]
> >>>>
> 
>