Review Request -- CR6565585: Performance improvements to Method.invoke(), Contrstuctor.newInstance() and Field.getFieldAccessor()
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
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
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
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
Mike Duigou wrote:
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
I think the changes look good and are low risk. Given that jdk7 is nearly done then it may be best to just push what you have and create another bug to track other suggestions, including Brian's idea to use a thread local cache. -Alan
Hi Mike, "needless critical section" is a bit of a vague way to describe what is fundamentally a change to the caching strategy :) From a synchronization perspective I concur that the new strategy is valid, but I can't comment on whether the right access check is actually being performed. In AccessibleObject.java, a minor nit, but place this comment: + // (Test cache[1] first since range check for [1] + // subsumes range check for [0].) before the array access rather than after it. Cheers, David Mike Duigou said the following on 03/18/11 09:04:
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
participants (4)
-
Alan Bateman
-
Brian Goetz
-
David Holmes
-
Mike Duigou