Claes - do you have thoughts on this performance difference?
Mandy
On 8/26/19 11:41 PM, Kazunori Ogata wrote:
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
