RFR: JDK-8229871: Improve performance of Method.copy() and leafCopy()
Kazunori Ogata
OGATAK at jp.ibm.com
Tue Oct 8 16:57:28 UTC 2019
Hi Claes,
Thank you for your review and comment.
I was also wondering why only these two fields aren't initialized in the
constructor. Shall I also make the change you mentioned?
Regards,
Ogata
Claes Redestad <claes.redestad at oracle.com> wrote on 2019/10/08 22:31:29:
> From: Claes Redestad <claes.redestad at oracle.com>
> To: Kazunori Ogata <OGATAK at jp.ibm.com>, core-libs-dev at openjdk.java.net
> Date: 2019/10/08 22:31
> Subject: [EXTERNAL] Re: RFR: JDK-8229871: Improve performance of
> Method.copy() and leafCopy()
>
> Hi,
>
> webrev.02 looks good to me.
>
> I think the performance results makes sense since avoiding a volatile
> store (and the potentially expensive store barriers this involves)
> should be the main benefit. Adding a branch to avoid storing null would
> help partially, but not hot Methods.
>
> Pre-existing issue, but it's somewhat weird that we have two assignments
> outside the package-private constructor: adding root and methodAccessor
> to the constructor would make more immediate sense to me, since we do
> the same thing at the only two callsites:
>
> Method res = new Method(clazz, name, parameterTypes,
returnType,
> exceptionTypes, modifiers, slot, signature,
> annotations, parameterAnnotations, annotationDefault);
> res.root = root;
> res.methodAccessor = methodAccessor;
>
> ->
>
> Method res = new Method(clazz, name, parameterTypes,
returnType,
> exceptionTypes, modifiers, slot, signature,
> annotations, parameterAnnotations, annotationDefault,
> root, methodAccessor);
>
> This package-private constructor could also be made private. My guess is
> there was some time when this was used from outside Method and changing
> it was deemed unsavory..
>
> Thanks!
>
> /Claes
>
> On 2019-10-08 12:23, 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.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
> >>>>>
> >>>>
> >>>>
> >
> >
>
More information about the core-libs-dev
mailing list