[
https://issues.apache.org/jira/browse/GROOVY-12263?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18105044#comment-18105044
]
ASF GitHub Bot commented on GROOVY-12263:
-----------------------------------------
daniellansun commented on code in PR #2790:
URL: https://github.com/apache/groovy/pull/2790#discussion_r3790866273
##########
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:
Agreed — thank you.
`invokeExact` only helps when the call site sees a stable `MethodType`. A
per-call `asSpreader` allocates a new handle, so that `invokeExact` was just a
more expensive `invoke`, and less readable than `invokeWithArguments`.
We took the other half of the same advice: the spreader is now built once in
`CallOverride.unreflect` and stored as `(Object, Object[])Object`
(`MethodType.genericMethodType(1, true)`). `invokeHandle`'s `default` is then:
```java
return handle.invokeExact((Object) self, arguments);
```
That is a cached spreader, so `invokeExact` is the type-correct call. We did
not use `handle.invokeWithArguments(self, arguments)`: that is a real varargs
method and would pack as length 2.
The specialised `0..4` `invokeExact` switch is unchanged. That remains the
GDK `each` / `collect` / `inject` path.
> 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)
