[
https://issues.apache.org/jira/browse/GROOVY-12284?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18106791#comment-18106791
]
ASF GitHub Bot commented on GROOVY-12284:
-----------------------------------------
blackdrag commented on code in PR #2822:
URL: https://github.com/apache/groovy/pull/2822#discussion_r3833632226
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
@@ -1533,4 +1533,42 @@ private static Class<?> getThisType(Class<?> sender) {
}
return sender;
}
+
+ /**
+ * Builds a same-class guard that avoids {@code
asCollector(Object[].class, n)}
+ * for the common 1–4 argument shapes (receiver plus 0–3 parameters).
+ * The produced handle has type {@code (pt)boolean} and returns {@code
false}
+ * if any argument is {@code null} or has a different runtime class.
+ */
+ static MethodHandle sameClassesGuard(final Object[] args, final Class<?>[]
pt) {
+ int n = pt.length;
+ MethodType booleanType = MethodType.methodType(boolean.class, pt);
+ if (n == 0) {
+ return MethodHandles.constant(boolean.class, true);
+ }
+ if (n == 1) {
+ return SAME_CLASS.bindTo(args[0].getClass()).asType(booleanType);
+ }
+ if (n == 2) {
+ return SAME_CLASSES_2
+ .bindTo(args[0].getClass()).bindTo(args[1].getClass())
+ .asType(booleanType);
+ }
+ if (n == 3) {
+ return SAME_CLASSES_3
+
.bindTo(args[0].getClass()).bindTo(args[1].getClass()).bindTo(args[2].getClass())
+ .asType(booleanType);
+ }
+ if (n == 4) {
+ return SAME_CLASSES_4
+ .bindTo(args[0].getClass()).bindTo(args[1].getClass())
+ .bindTo(args[2].getClass()).bindTo(args[3].getClass())
+ .asType(booleanType);
+ }
+ Class<?>[] classes = new Class<?>[n];
+ for (int i = 0; i < n; i++) {
+ classes[i] = args[i].getClass();
+ }
+ return SAME_CLASSES.bindTo(classes).asCollector(Object[].class,
n).asType(booleanType);
+ }
Review Comment:
I think you should look if this is not better by using `insertArguments`
then you could do:
```
int n = pt.length;
if (n==0) return MethodHandles.constant(boolean.class, true);
Class<?>[] classes = new Class<?>[n];
for (int i = 0; i < n; i++) {
classes[i] = args[i].getClass();
}
if (n<5) {
return MethodHandles.insertArguments(sameClassGuards[n], 0,
classes).asType(booleanType);
}
return SAME_CLASSES.bindTo(classes).asCollector(Object[].class,
n).asType(booleanType);
```
`sameClassGuards` is a simple helper array that stores the SAME_CLASSES_*
guards according to their arity. `insertArgument` should then make for example
out of (Class,Class,Object,Object): boolean a (Object,Object):boolean. I could
imagine that a smaller lambda form for a single insertArguments compared to
multiple bindTo. But I have no proof for it, or maybe it does not matter.
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
@@ -1314,11 +1317,8 @@ public void setGuards(Object receiver) {
handle = MethodHandles.guardWithTest(test, handle,
fallback);
}
} else if (Arrays.stream(pt).anyMatch(nonFinalOrNullUnsafe)) {
- MethodHandle test = SAME_CLASSES
-
.bindTo(Arrays.stream(args).map(Object::getClass).toArray(Class[]::new))
- .asCollector(Object[].class, pt.length)
- .asType(MethodType.methodType(boolean.class, pt));
- handle = MethodHandles.guardWithTest(test, handle, fallback);
+ handle = MethodHandles.guardWithTest(
+ Selector.sameClassesGuard(args, pt), handle, fallback);
Review Comment:
I would skip the guard completely for the "no arguments"-case.
You could also consider not giving pt in, but handle.type and then use
`MethodType.changeReturnType` inside to produce the guard signature. Or you do
it directly here and give the guard target signature in instead. not sure about
the performance impact. It will probably not have a big impact, unless this
somehow is using a faster path in the JDK to produce the costly MethodType.
That also gives me another idea which is maybe out of the scope of this
PR.if we would have a number for the callsites like the callsite array had, we
could actually store such forms, that depend on the callsite in a ClassValue
for the hosting class. It could be an array, index the callsite index, where we
then get the form from. Well, maybe too big a change - no idea if that would
actually improve the situation, since you also have to pay for the additional
argument.
> Specialize indy sameClasses guards for arity 1-4
> ------------------------------------------------
>
> Key: GROOVY-12284
> URL: https://issues.apache.org/jira/browse/GROOVY-12284
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
>
> h3. Problem
> When an invokedynamic site is linked with all arguments non-null and at least
> one parameter type that is non-final (or a primitive wrapper — GROOVY-11782),
> {{Selector}} installs a same-class guard:
> {code:java}
> SAME_CLASSES
> .bindTo(expectedClasses)
> .asCollector(Object[].class, n)
> .asType(MethodType.methodType(boolean.class, pt));
> {code}
> {{asCollector}} of an {{Object}} array of length {{n}} allocates a fresh
> array on *every later invocation* of that site; the array overload of
> {{sameClasses}} then walks it.
> That is the hot path for ordinary dynamic Groovy calls of the shapes
> {{recv.foo()}}, {{recv.foo(a)}}, {{recv.foo(a, b)}}, {{recv.foo(a, b, c)}} —
> arity 1-4 (receiver plus 0-3 arguments). Dynamic indy sites almost always
> have {{Object}} parameter types, so this guard is the common case, not a rare
> fallback.
> (If any argument is {{null}} at link time, {{Selector}} already installs
> per-slot {{SAME_CLASS}} / {{IS_NULL}} tests and does not use the collector.)
> The classic MOP already specializes this check:
> {{MetaClassHelper.sameClasses}} has overloads for 0-4 arguments so the
> call-site cache does not box arguments into an array. The indy guard did not.
> h3. Goal
> Keep the same guard semantics (return {{false}} if any argument is {{null}}
> or has a different runtime class) without allocating an {{Object}} array on
> the common 1-4 arity shapes.
> h3. Approach
> ||Arity (incl. receiver)||Guard||
> |0|constant {{true}}|
> |1|existing {{SAME_CLASS}}|
> |2|new {{SAME_CLASSES_2}}|
> |3|new {{SAME_CLASSES_3}}|
> |4|new {{SAME_CLASSES_4}}|
> |5 or more|existing {{SAME_CLASSES}} plus {{asCollector}} (unchanged)|
> Expected classes are bound with {{bindTo}}. One {{guardWithTest}} at the
> site, via a single {{Selector.sameClassesGuard(args, pt)}} helper.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
