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

[Peter Levart]

Hi Ogata,

May I just add that volatile field ensured that MethodAccessor object 
was correctly published. DelegatingMethodAccessortImpl is not safe to be 
published via data race because it contains plain `delegate` field 
initialized in the constructor. In addition, the object that is first 
assigned to that field is NativeMethodAccessorImpl which has plain 
`parent` field. You can get NPE when invoking the Method.invoke from 
multuiple threads if Method.methodAccessor is not volatile.

In addition, It would be nice to have two microbenchmarks exercising:
a) Method copy performance
b) Method invocation performance

Regards, Peter

P.S. When exploring the possibility of an alternative MethodAccessor 
implementation (using MethodHandle(s)):

http://cr.openjdk.java.net/~plevart/jdk-dev/6824466_MHReflectionAccessors/webrev.00.2/

...I found out that it was possible to re-arrange the play between 
DelegatingMethodAccessorImpl, NativeMethodAccessorImpl and generated 
MethodAccessor in such a way that the DelegatingMethodAccessortImpl 
becomes safe to be published via data race. This allowed for 
Method.methodAccessor field to become plain field. In addition this 
field can be made @Stable which further optimizes access to 
MethodAccessor instance when Method instance can be constant-folded, 
which showed in special microbenchmarks.

So perhaps you could try to use above implementation (just changes to 
DelegatingMethodAccessorImpl, NativeMethodAccessorImpl and part of 
Reflection factory but without MH* stuff) and measure it against current 
and your implementation (which as shown above has a data-race flaw).

On 10/8/19 12:23 PM, 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
>>>>>
>>>>
>