[Performance] Use invoke dynamic instead of Method.invoke for @Exported #96
Conversation
i386
changed the title
|
PTAL @vivek |
| @@ -13,7 +13,7 @@ pipeline { | |||
| } | |||
| } | |||
|
|
|||
| postBuild { | |||
| post { | |||
There was a problem hiding this comment.
Unrelated change to make CI pass. Needs #93 to be merged.
| return METHOD_HANDLES.getUnchecked(method); | ||
| } | ||
|
|
||
| private static final LoadingCache<Method, MethodHandle> METHOD_HANDLES = CacheBuilder.newBuilder().expireAfterAccess(1, TimeUnit.HOURS).build(new CacheLoader<Method, MethodHandle>() { |
There was a problem hiding this comment.
Maybe configurable number of cache expiry hours, maybe a system property?
|
@i386 It's surprising to see this kind of perf gain just by switch to invokedynamic based invocation. these are pretty basic method invocation (no duck typing involved really, unless we are talking about groovy or jruby like working with duck typing). Or maybe its the cache doing the perf gain as compared to invoke dynamic? IOW did you try comparing perf gain without caching? |
|
@vivek most of the gains are made when Blue Ocean is using stapler's export package as each @exported method gets called with reflection. For Blue Ocean, there is a substantial amount of |
|
@i386 Thanks, tested that code locally, indeed MethodHandle speeds things up compared to reflection for large number of iterations. Your changes LGTM 🐝 |
|
@vivek did some testing here for 20 mins - it seems the more invocations we make the more the JVM can optimise them. |
|
Test failure is unrelated and intermittent |
|
With that change, I don't think caching of I'm happy to make relevant changes and merge. I think it's also fine at this point to require Java7, and hopefully that'll drive some additional source code changes. |
|
@kohsuke ok that sounds great to me! Appreciate you reviewing these changes 👌 |
…c method, as in the case with Klass.getFunctions() on a non-public class
The standard procedure is to either request that the original filer do those changes, or to create a new PR subsuming the original one and adding further commits. In either case a fresh round of reviews is required. |
Use invoke dynamic (
MethodHandles) instead of Method.invoke for @exported.MethodHandlesseem to be what all the cool serialisers like protobuf use for performance reasons. This means that Stapler has to require JDK 7 minimum.Initial testing seemed to show I could shave 20% off of the time to serve some Blue Ocean rest calls.