RFR: JDK-8229871: Improve performance of Method.copy() and leafCopy()
Kazunori Ogata
OGATAK at jp.ibm.com
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
> > > >
> > >
> > >
More information about the core-libs-dev
mailing list