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 > > > > > > > > > >
