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

[Kazunori Ogata]

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