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

[Kazunori Ogata]

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