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