Hi Peter, Thank you for the comment and suggestion of the fix.
I tried to pick up your change w.r.t. methodAccessor: https://cr.openjdk.java.net/~ogatak/8229871/webrev.04/ Regarding micro benchmark, my original motivation of this change is to improve performance of Class.getMethods(), which calls Method.copy() for each declared method to create a copy of Method[]. I measured my simple microbench: https://cr.openjdk.java.net/~ogatak/8229871/GetMethodsBench.java Base code: Elapsed time = 4808 ms webrev.01: Elapsed time = 4536 ms (+ 6%) webrev.02: Elapsed time = 2331 ms (+106%) webrev.04: Elapsed time = 3746 ms (+ 28%) I'll measure larger benchmark and try to think if we can reduce the overhead of memory barrier. Regards, Ogata Peter Levart <peter.lev...@gmail.com> wrote on 2019/10/09 16:44:13: > From: Peter Levart <peter.lev...@gmail.com> > To: Kazunori Ogata <oga...@jp.ibm.com>, core-libs-dev@openjdk.java.net > Date: 2019/10/09 16:44 > Subject: [EXTERNAL] Re: RFR: JDK-8229871: Improve performance of > Method.copy() and leafCopy() > > Hi Ogata, > > May I just add that volatile field ensured that MethodAccessor object was > correctly published. DelegatingMethodAccessortImpl is not safe to be > published via data race because it contains plain `delegate` field > initialized in the constructor. In addition, the object that is first > assigned to that field is NativeMethodAccessorImpl which has plain > `parent` field. You can get NPE when invoking the Method.invoke from > multuiple threads if Method.methodAccessor is not volatile. > > In addition, It would be nice to have two microbenchmarks exercising: > a) Method copy performance > b) Method invocation performance > > Regards, Peter > > P.S. When exploring the possibility of an alternative MethodAccessor > implementation (using MethodHandle(s)): > > http://cr.openjdk.java.net/~plevart/jdk-dev/6824466_MHReflectionAccessors/ > webrev.00.2/ > > ...I found out that it was possible to re-arrange the play between > DelegatingMethodAccessorImpl, NativeMethodAccessorImpl and generated > MethodAccessor in such a way that the DelegatingMethodAccessortImpl > becomes safe to be published via data race. This allowed for > Method.methodAccessor field to become plain field. In addition this field > can be made @Stable which further optimizes access to MethodAccessor > instance when Method instance can be constant-folded, which showed in > special microbenchmarks. > > So perhaps you could try to use above implementation (just changes to > DelegatingMethodAccessorImpl, NativeMethodAccessorImpl and part of > Reflection factory but without MH* stuff) and measure it against current > and your implementation (which as shown above has a data-race flaw). > On 10/8/19 12:23 PM, 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 > > > > >
