core-libs-dev Digest, Vol 47, Issue 35

[Guy Korland]

It seems like a great improvement that can help many frameworks.
Is there any plan to port the fix to JDK6?

Thanks,
Guy

________________________________________________________________________________
Date: Mon, 21 Mar 2011 16:29:22 -0700
From: Mike Duigou <mike.duigou at oracle.com>
Subject: Re: Review Request -- CR6565585: Performance improvements to
       Method.invoke(),        Contrstuctor.newInstance() and
       Field.getFieldAccessor()
To: Brian Goetz <brian.goetz at oracle.com>
Cc: core-libs-dev at openjdk.java.net
Message-ID: <F47C38AB-123A-4620-82F2-B6062373DBCC at oracle.com>
Content-Type: text/plain; charset=us-ascii

I've updated the patch to avoid the second read-volatile by having the
acquire* methods return a result. Method already used that approach.

I am less certain about tackling additional improvements to the caching in
this patch. In part because I suspect that the usage patterns for
Method.invoke(), Constructor.newInstance() and Field.get*() are different
and I don't have any metrics which I can reference to say which is the
appropriate strategy for each. My suspicion is the usage pattern for Field
differs significantly from Constructor and Method which are likely more
similar. If anyone can contribute usage pattern experiences or practical
insights into how these are then we can come up with appropriate solutions.

The update webrev : http://cr.openjdk.java.net/~mduigou/6565585/1/webrev/

Mike

On Mar 17 2011, at 16:26 , Brian Goetz wrote:

> You might consider storing the cached item in checkAccess() in a
ThreadLocal instead of a static.  Not only would this eliminate the volatile
accesses, but it reduces the chance of a "miss" in this one-element cache,
since it will then be impervious to interleavings where other threads might
use reflection.  (The purpose of the cache is to speed up the case when some
code is grinding through all the members of a Class; it is unlikely that
there would be many hits in this cache across threads anyway.)  Obviously
this is a tradeoff of memory utilization for reduced synchronization traffic
+ higher cache hit rate + more predictable reflection performance under
concurrent load.
>
> You can eliminate the second read-volatile by having acquireMethodAccessor
return the MA instead of re-reading the volatile.
>
> On 3/17/2011 7:04 PM, Mike Duigou wrote:
>> Sorry folks--the webrev url:
http://cr.openjdk.java.net/~mduigou/6565585/0/webrev/
>>
>> Mike
>>
>> On Mar 17 2011, at 15:07 , Mike Duigou wrote:
>>
>>> Method.invoke(), Contrstuctor.newInstance() and Field.getFieldAccessor()
all have a needless critical section, causing large slowdowns. This patch a
replaces the synchronizations by volatile references. Finally, the changes
remove a doubled reference to another volatile variable.  This also
simplifies the generated code by commoning up the corresponding load
instruction used in the fast execution path.
>>>
>>> Speedups from this change are uniformly 2x or better.
>>>
>>> The proposed improvement and patch was originated by John Rose.
>>>
>>> Thanks,
>>>
>>> Mike
>>