[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-core-libs-dev
Subject:    Re: RFR: JDK-8229871: Improve performance of Method.copy() and leafCopy()
From:       Mandy Chung <mandy.chung () oracle ! com>
Date:       2019-08-27 14:34:11
Message-ID: 46930b4e-fd48-4638-c2bb-ecde19fe6296 () oracle ! com
[Download RAW message or body]

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.chung@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.chung@oracle.com> wrote on 2019/08/21 01:21:42:
>>
>>> From: Mandy Chung <mandy.chung@oracle.com>
>>> To: Kazunori Ogata <OGATAK@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
>>>>
>>>
>

[prev in list] [next in list] [prev in thread] [next in thread]