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
