[
https://issues.apache.org/jira/browse/GROOVY-12263?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18105039#comment-18105039
]
ASF GitHub Bot commented on GROOVY-12263:
-----------------------------------------
blackdrag commented on code in PR #2790:
URL: https://github.com/apache/groovy/pull/2790#discussion_r3790709178
##########
src/main/java/groovy/lang/Closure.java:
##########
@@ -594,6 +586,59 @@ public V call(final Object... arguments) {
}
}
+ /**
+ * Invokes a cached {@code doCall}/{@code call} target. Prefers the adapted
+ * {@link MethodHandle} so {@code Method.invoke} is not on the GDK
+ * {@code each}/{@code collect} hot path. Exceptions thrown by the body —
+ * including a body that itself throws {@link InvocationTargetException} or
+ * {@link IllegalAccessException} — are rethrown as-is on the handle path;
+ * the {@link Method#invoke} fallback unwraps only the wrapper
+ * {@link InvocationTargetException} that reflection introduces.
+ */
+ @SuppressWarnings("unchecked")
+ private static <V> V invokeCached(final MethodHandle handle, final Method
target, final Closure<?> self, final Object[] arguments) {
+ if (handle != null) {
+ try {
+ return (V) invokeHandle(handle, self, arguments);
+ } catch (Throwable t) {
+ UncheckedThrow.rethrow(t);
+ return null;
+ }
+ }
+ try {
+ return (V) target.invoke(self, arguments);
+ } catch (InvocationTargetException ite) {
+ UncheckedThrow.rethrow(ite.getCause());
+ return null; // unreachable statement
+ } catch (IllegalAccessException iae) {
+ throw new GroovyRuntimeException(iae);
+ }
+ }
+
+ /**
+ * {@code invokeExact} against a handle adapted to
+ * {@link MethodType#genericMethodType(int) genericMethodType(arity+1)}
+ * (fixed-arity {@code Object} receiver and arguments, {@code Object}
return).
+ * Cases {@code 0..ARITY_LIMIT-1} match that type exactly; the spreader
+ * is the type-correct fallback if the limit grows without a matching case.
+ */
+ private static Object invokeHandle(final MethodHandle handle, final
Closure<?> self, final Object[] arguments) throws Throwable {
+ switch (arguments.length) {
+ case 0:
+ return handle.invokeExact((Object) self);
+ case 1:
+ return handle.invokeExact((Object) self, arguments[0]);
+ case 2:
+ return handle.invokeExact((Object) self, arguments[0],
arguments[1]);
+ case 3:
+ return handle.invokeExact((Object) self, arguments[0],
arguments[1], arguments[2]);
+ case 4:
+ return handle.invokeExact((Object) self, arguments[0],
arguments[1], arguments[2], arguments[3]);
+ default:
+ return handle.asSpreader(Object[].class,
arguments.length).invokeExact((Object) self, arguments);
Review Comment:
I think you have no advantage of invokeExact in this case, unless you cache
the handle resulting from asSpreader. If not cached I would use
invokeWithArguments here instead. I doubt it is slower for this case, but it
sure is better readable.
##########
src/main/java/groovy/lang/Closure.java:
##########
@@ -1593,7 +1650,26 @@ static CallOverride lookup(Class<?> type) {
}
any = true;
}
- return any ? new CallOverride(byArity, guards, callForm) : NONE;
+ if (!any) {
+ return NONE;
+ }
+ MethodHandle[] handles = new MethodHandle[ARITY_LIMIT];
+ for (int arity = 0; arity < ARITY_LIMIT; arity += 1) {
+ if (byArity[arity] != null) {
+ handles[arity] = unreflect(byArity[arity]);
+ }
+ }
Review Comment:
the cases over arity limit could be created here as well, but with taking
Object[]
> Invoke cached Closure doCall targets via MethodHandle
> -----------------------------------------------------
>
> Key: GROOVY-12263
> URL: https://issues.apache.org/jira/browse/GROOVY-12263
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
>
> The {{Closure.call(Object...)}} fast path (GROOVY-11911, GROOVY-12164,
> GROOVY-12165) already caches a per-arity {{doCall}} / {{call}} {{Method}} and
> invokes it with {{Method.invoke}}. That still puts a reflective invoke —
> access check, argument boxing, {{InvocationTargetException}} wrap — on every
> GDK {{each}} / {{collect}} / {{findAll}} / {{inject}} callback from Java.
> h2. Proposal
> At cache-build time, {{MethodHandles.unreflect}} the cached {{Method}} and
> adapt it to {{genericMethodType(arity+1)}}. {{call(Object...)}} then prefers
> {{invokeExact}} on that handle (specialized for arities 0–4).
> {{Method.invoke}} remains only when the method cannot be adapted, so the
> GROOVY-11911 {{call()}} / {{call(Object)}} carve-out still works if unreflect
> fails.
> Exception contracts stay as they were on the reflective path: a body-thrown
> throwable surfaces unwrapped. The handle path must not treat a body-thrown
> {{InvocationTargetException}} or {{IllegalAccessException}} as a reflection
> wrapper.
> Guards, {{CallOverride.NONE}} for {{MethodClosure}} / {{CurriedClosure}}, and
> the metaclass fallback for coercion (GROOVY-12164) are unchanged.
> h2. Why this path
> Java callers such as {{DefaultGroovyMethods}} resolve {{closure.call(item)}}
> to {{Closure.call(Object)}}, which wraps into {{call(Object...)}}. Groovy
> {{invokedynamic}} sites typically bind straight to {{doCall}} after warmup
> and never enter this method — they are out of scope.
> h2. Verification
> Same-host JMH, 4 forks, 99.9% CI, parent {{9bb195dee5}} vs {{1c3820bff7}},
> JDK 25. Host-calibration geomean 0.998x.
> || bench || speedup ||
> | {{eachWithClosure}} | 1.171x |
> | {{collectWithClosure}} | 1.163x |
> | {{findAllWithClosure}} | 1.128x |
> | {{injectWithClosure}} | 1.212x |
> | GDK geomean | 1.168x (~5 ns/callback) |
> | Groovy-indy {{doCall}} sites | 0.990x (flat) |
> | {{MethodClosure}} ({{list.&size}}) | 0.994x (flat) |
> All four GDK 99.9% CIs are disjoint. Full write-up:
> {{docs/closure-call-methodhandle-perf-report.md}}.
> h2. Related
> GROOVY-11911 introduced the reflective cache. GROOVY-12164 / GROOVY-12165
> extended it with typed and multi-arity guards. This change keeps that
> selection and only replaces the invoke.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
