Hi Claes, Thank you for your review and comment.
I was also wondering why only these two fields aren't initialized in the constructor. Shall I also make the change you mentioned? Regards, Ogata Claes Redestad <claes.redes...@oracle.com> wrote on 2019/10/08 22:31:29: > From: Claes Redestad <claes.redes...@oracle.com> > To: Kazunori Ogata <oga...@jp.ibm.com>, core-libs-dev@openjdk.java.net > Date: 2019/10/08 22:31 > Subject: [EXTERNAL] Re: RFR: JDK-8229871: Improve performance of > Method.copy() and leafCopy() > > Hi, > > webrev.02 looks good to me. > > I think the performance results makes sense since avoiding a volatile > store (and the potentially expensive store barriers this involves) > should be the main benefit. Adding a branch to avoid storing null would > help partially, but not hot Methods. > > Pre-existing issue, but it's somewhat weird that we have two assignments > outside the package-private constructor: adding root and methodAccessor > to the constructor would make more immediate sense to me, since we do > the same thing at the only two callsites: > > Method res = new Method(clazz, name, parameterTypes, returnType, > exceptionTypes, modifiers, slot, signature, > annotations, parameterAnnotations, annotationDefault); > res.root = root; > res.methodAccessor = methodAccessor; > > -> > > Method res = new Method(clazz, name, parameterTypes, returnType, > exceptionTypes, modifiers, slot, signature, > annotations, parameterAnnotations, annotationDefault, > root, methodAccessor); > > This package-private constructor could also be made private. My guess is > there was some time when this was used from outside Method and changing > it was deemed unsavory.. > > Thanks! > > /Claes > > On 2019-10-08 12:23, Kazunori Ogata wrote: > > Hi all, > > > > I posted two changes and got reply that performance evaluation is needed. > > I found that making Method.methodAccessor non-volatile (webrev.02) is > > better than avoid copying methodAccessor value when it is null > > (webrev.01), as shown below. > > > > So I'd like to ask review of the former change. I updated weberv using > > the latest code base (though there was no difference from webrev.02): > > > > Webrev: http://cr.openjdk.java.net/~ogatak/8229871/webrev.03/ > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8229871 > > > > > > For this performance evaluation, I calculated 75 percentile of 9 runs of > > SPECjbb2015 and 60 percentile of 50 runs of DaCapo to omit outliers. I > > bound a JVM to a NUMA node and set the number of GC threads to the same as > > the number of physical cores. These tuning reduced run-to-run fluctuation > > on POWER (as usual...). > > > > SPECjbb2015: > > webrev.02: critical jOPS +1.6%, max jOPS +0.2% > > webrev.01: critical jOPS +0.4%, max jOPS +0.2% > > > > > > For DaCapo, some benchmark still improved performance and some degraded, > > but the geometric mean of all benchmarks were small: > > weberv.02: +0.3% > > weberv.01: +0.2% > > > > The difference of improvement/degradation between the two changes in each > > benchmark were less than 0.8%. > > > > The range of improvement/degradation in each benchmark were: > > webrev.02: between +2.4% and -1.0% > > webrev.01: between +1.6% and -1.8% > > > > > > So I think webrev.02 (i.e., making methodAccessor non-volatile) is a good > > change, since it improved SPECjbb critical jOPS by 1.6%. > > > > > > Regards, > > Ogata > > > > > > Kazunori Ogata/Japan/IBM wrote on 2019/08/27 15:41:39: > > > >> From: Kazunori Ogata/Japan/IBM > >> To: Mandy Chung <mandy.ch...@oracle.com> > >> Cc: core-libs-dev@openjdk.java.net > >> Date: 2019/08/27 15:41 > >> Subject: Re: RFR: JDK-8229871: Improve performance of Method.copy() and > > leafCopy() > >> > >> Hi Mandy, > >> > >> Let me post interim results of the performance evaluation, though I'm > >> still measuring benchmarks and analyzing them. > >> > >> For SPECjbb2015, skipping storing null (webrev.01) was faster than > > making > >> methodAccessor non-volatile (webrev.02). The improvements of each of > > the > >> patches in maxJOPS/criticalJOPS were 2.6%/3.9% and 1.8%/2.9%, > >> respectively. This is only an average of six runs. > >> > >> For DaCapo, the results were mixed. In some benchmark, both of the > >> changes degraded performance. In some others, webrev.01 was better, but > > > >> weberv.02 was better in some others. > >> > >> I'll continue evaluation, but it is helpful if you could give me some > >> hints on why webrev.01 can be better than webrev.02 in SPECjbb2015. > >> > >> Regards, > >> Ogata > >> > >> Kazunori Ogata/Japan/IBM wrote on 2019/08/21 20:02:41: > >> > >>> From: Kazunori Ogata/Japan/IBM > >>> To: Mandy Chung <mandy.ch...@oracle.com> > >>> Cc: core-libs-dev@openjdk.java.net > >>> Date: 2019/08/21 20:02 > >>> Subject: Re: RFR: JDK-8229871: Improve performance of Method.copy() > > and leafCopy() > >>> > >>> 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 > >>>>> > >>>> > >>>> > > > > >
