Hi Mandy, Thank you for reviewing the webrev. I updated it to add a space after "if" and also put four spaces for indentation (it was three).
http://cr.openjdk.java.net/~ogatak/8229871/webrev.01/ Thank you so much for checking the history of fieldAccessor. I was surprised that fieldAccessor was made non-volatile in JDK5, but methodAccessor was left as volatile for 15 years after that... I agree we need benchmark data. My simple micro benchmark that repeats invoking Class.getMethods() improved performance by 70% when it made non-volatile (as shown in the following webrev). I'll try to run larger benchmarks, such as SPECjbb2015, to see real impact. http://cr.openjdk.java.net/~ogatak/8229871/webrev.02/ Regards, Ogata Mandy Chung <mandy.ch...@oracle.com> wrote on 2019/08/21 01:21:42: > From: Mandy Chung <mandy.ch...@oracle.com> > To: Kazunori Ogata <oga...@jp.ibm.com> > Cc: core-libs-dev@openjdk.java.net > Date: 2019/08/21 01:21 > Subject: [EXTERNAL] Re: RFR: JDK-8229871: Imporve performance of > Method.copy() and leafCopy() > > Hi Ogata, > > The patch looks okay. Nit: please add a space between if and (. > > About volatile methodAccessor field, I checked the history. Both > fieldAccessor and methodAccessor were started as volatile and the > fieldAccessor declaration was updated due to JDK-5044412. As you > observe, I think the methodAccessor field could be made non-volatile. > OTOH that might impact when it's inflated to spin bytecode for this > method invocation. I don't know how importance to keep its volatile vs > non-volatile in practice without doing benchmarking/real application > testing. > > Mandy > > On 8/19/19 2:51 AM, Kazunori Ogata wrote: > > Hi, > > > > May I have review for "JDK-8229871: Imporve performance of Method.copy() > > and leafCopy()"? > > > > Method.copy() and leafCopy() creates a copy of a Method object with > > sharing MethodAccessor object. Since the methodAccessor field is a > > volatile variable, copying this field needs memory fence to ensure the > > field is visible to all threads on the weak memory platforms such as POWER > > and ARM. > > > > When the methodAccessor of the root object is null (i.e., not initialized > > yet), we do not need to copy the null value because this field of the > > copied object has been initialized to null in the constructor. We can > > reduce overhead of the memory fence only when the root's methodAccessor is > > non-null. This change improved performance by 5.8% using a micro benchmark > > that repeatedly invokes Class.getMethods(). > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8229871 > > > > Webrev: http://cr.openjdk.java.net/~ogatak/8229871/webrev.00/ > > > > > > By the way, why Method.methodAccessor is volatile, while > > Field.fieldAccessor and Field.overrideFieldAccessor are not volatile? I > > know the use of volatile reduces probability of creating duplicated method > > accessor, but the chance still exists. I couldn't find the difference > > between Method and Field classes to make Method.methodAccessor volatile. > > If we can make it non-volatile, it is more preferable than a quick hack > > above. > > > > > > Regards, > > Ogata > > > >
