RFR: JDK-8229871: Improve performance of Method.copy() and leafCopy()

Tue Oct 8 10:23:01 UTC 2019

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.chung at oracle.com>
> Cc: core-libs-dev at 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.chung at oracle.com>
> > Cc: core-libs-dev at 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.chung at oracle.com> wrote on 2019/08/21 01:21:42:
> > 
> > > From: Mandy Chung <mandy.chung at oracle.com>
> > > To: Kazunori Ogata <OGATAK at jp.ibm.com>
> > > Cc: core-libs-dev at 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
> > > >
> > > 
> > > 