Re: Re: RFR: JDK-8227831: Avoid using volatile for write-once, read-many class field
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" wrote on 2019/07/19 02:58:50: > From: Mandy Chung > To: Claes Redestad , David Holmes > > 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" > > 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 > >>
Re: RFR: JDK-8227831: Avoid using volatile for write-once, read-many class field
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 wrote on 2019/07/17 16:49:08: From: Aleksey Shipilev To: Kazunori Ogata , 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]
Re: RFR: JDK-8227831: Avoid using volatile for write-once, read-many class field
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 wrote on 2019/07/17 16:49:08: From: Aleksey Shipilev To: Kazunori Ogata , 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]
Re: RFR: JDK-8227831: Avoid using volatile for write-once, read-many class field
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. 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 wrote on 2019/07/17 16:49:08: From: Aleksey Shipilev To: Kazunori Ogata , 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]
Re: RFR: JDK-8227831: Avoid using volatile for write-once, read-many class field
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. 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. 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 wrote on 2019/07/17 16:49:08: From: Aleksey Shipilev To: Kazunori Ogata , 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]
Re: RFR: JDK-8227831: Avoid using volatile for write-once, read-many class field
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 wrote on 2019/07/17 16:49:08: > From: Aleksey Shipilev > To: Kazunori Ogata , 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]
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
RE: RFR: JDK-8227831: Avoid using volatile for write-once, read-many class field
Hi Ogata,
this seems to make sense. So, +1 from my end.
Can you please add a space after "if" in line "734
if(langReflectAccess == null) {"?
Thanks
Christoph
> -Original Message-
> From: core-libs-dev On Behalf
> Of Kazunori Ogata
> Sent: Mittwoch, 17. Juli 2019 08:49
> To: core-libs-dev@openjdk.java.net
> Subject: RFR: JDK-8227831: Avoid using volatile for write-once, read-many
> class field
>
> Hi,
>
> May I have a review for "JDK-8227831: Avoid using volatile for write-once,
> read-many class field"?
>
> In jdk.internal.reflect.ReflectionFactory, there is a private class field
> named "langReflectAccess", which is referenced every time when the library
> handles various reflective operations. This field is initialized on the
> first access to the ReflectionFactory class. This field is declared as
> volatile to avoid (or reduce) race condition between initialization and
> references to the field.
>
> On the platforms with weak memory model (i.e, POWER and ARM), reading a
> volatile variable requires memory fence and incurs overhead. So it is
> preferable to avoid use of volatile for such a write-once, read-many
> variable.
>
> langReflectAccess can be modified only in setLangReflectAccess() method.
> So we can avoid using volatile by modifying setLangReflectAccess() to use
> a synchronized block to avoid race condition. This change reduced elapsed
> time of a micro benchmark by 9%, which repeatedly invoke
> Class.getMethods().
>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8227831
>
> Webrev: http://cr.openjdk.java.net/~ogatak/8227831/webrev/
>
>
> Regards,
> Ogata
RFR: JDK-8227831: Avoid using volatile for write-once, read-many class field
Hi, May I have a review for "JDK-8227831: Avoid using volatile for write-once, read-many class field"? In jdk.internal.reflect.ReflectionFactory, there is a private class field named "langReflectAccess", which is referenced every time when the library handles various reflective operations. This field is initialized on the first access to the ReflectionFactory class. This field is declared as volatile to avoid (or reduce) race condition between initialization and references to the field. On the platforms with weak memory model (i.e, POWER and ARM), reading a volatile variable requires memory fence and incurs overhead. So it is preferable to avoid use of volatile for such a write-once, read-many variable. langReflectAccess can be modified only in setLangReflectAccess() method. So we can avoid using volatile by modifying setLangReflectAccess() to use a synchronized block to avoid race condition. This change reduced elapsed time of a micro benchmark by 9%, which repeatedly invoke Class.getMethods(). Bug: https://bugs.openjdk.java.net/browse/JDK-8227831 Webrev: http://cr.openjdk.java.net/~ogatak/8227831/webrev/ Regards, Ogata
