You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by "paulk-asert (via GitHub)" <gi...@apache.org> on 2026/07/07 10:33:52 UTC
[PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
paulk-asert opened a new pull request, #2673:
URL: https://github.com/apache/groovy/pull/2673
…ike)
Behind the groovy.indy.cold.reflection flag (default: off), plain method calls dispatch reflectively while a call site is cold, deferring all MethodHandle chain construction — and its one-time LambdaForm creation cost — to hit-count promotion. The shared cold dispatcher has a single (Object[])Object shape for every call site, arity, and primitive pattern; promotion installs the unchanged full guarded chain, so the hot path is identical to master.
Plain calls are public, non-static, non-category CachedMethods on public classes with a real receiver and no spread/safe-null/interceptor semantics; caller-sensitive targets (including unannotated caller-context-sensitive cases like object serialization, and interface selections resolving to sensitive implementations) stay on the full path. Selection reuses Selector's existing logic via a new selection-only hook; validity is re-checked per call with plain-Java equivalents of the chain's guards. A per-wrapper cumulative counter promotes polymorphic receiver shapes that the consecutive-hit counter never catches.
Measured (deterministic class-load counters + fresh-JVM timing): ~2/3 of per-shape LambdaForm cost removed on cold dispatch workloads, 1.34-1.40x faster cold dispatch with many cold sites, steady state unchanged after promotion, MOP-heavy test subset (3437 tests) green with the flag on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on code in PR #2673:
URL: https://github.com/apache/groovy/pull/2673#discussion_r3549141822
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
@@ -1226,6 +1251,35 @@ void buildInvokeHandle() {
addExceptionHandler();
}
}
+
+ @Override
+ MetaMethod selectForColdReflection() {
+ // defensive: the cold tier is only for plain method calls; the
+ // fallback gates on CallType.METHOD, so subclasses (CastSelector,
+ // InterfaceSelector) are never asked, but make it explicit here
+ if (callType != CallType.METHOD) return null;
Review Comment:
(GroovyBugError): Agreed and done — reaching this with a non-METHOD call type would mean the fallback's gate broke, so it now throws GroovyBugError.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2673:
URL: https://github.com/apache/groovy/pull/2673#issuecomment-4903144786
## [Codecov](https://app.codecov.io/gh/apache/groovy/pull/2673?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `68 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 68.6088%. Comparing base ([`c6fa475`](https://app.codecov.io/gh/apache/groovy/commit/c6fa475e9eef061c186d88e0fc3079241ff50ed2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`8b41cd8`](https://app.codecov.io/gh/apache/groovy/commit/8b41cd8df68e9e569771fe70c37b8dec9cdf8826?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/groovy/pull/2673?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...vmplugin/v8/ColdReflectiveMethodHandleWrapper.java](https://app.codecov.io/gh/apache/groovy/pull/2673?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fvmplugin%2Fv8%2FColdReflectiveMethodHandleWrapper.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3ZtcGx1Z2luL3Y4L0NvbGRSZWZsZWN0aXZlTWV0aG9kSGFuZGxlV3JhcHBlci5qYXZh) | 0.0000% | [56 Missing :warning: ](https://app.codecov.io/gh/apache/groovy/pull/2673?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...java/org/codehaus/groovy/vmplugin/v8/Selector.java](https://app.codecov.io/gh/apache/groovy/pull/2673?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fvmplugin%2Fv8%2FSelector.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3ZtcGx1Z2luL3Y4L1NlbGVjdG9yLmphdmE=) | 0.0000% | [12 Missing :warning: ](https://app.codecov.io/gh/apache/groovy/pull/2673?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2673?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2673 +/- ##
==================================================
+ Coverage 68.5934% 68.6088% +0.0154%
- Complexity 33951 34094 +143
==================================================
Files 1535 1537 +2
Lines 128543 128998 +455
Branches 23335 23393 +58
==================================================
+ Hits 88172 88504 +332
- Misses 32555 32661 +106
- Partials 7816 7833 +17
```
| [Files with missing lines](https://app.codecov.io/gh/apache/groovy/pull/2673?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...org/codehaus/groovy/vmplugin/v8/IndyInterface.java](https://app.codecov.io/gh/apache/groovy/pull/2673?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fvmplugin%2Fv8%2FIndyInterface.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3ZtcGx1Z2luL3Y4L0luZHlJbnRlcmZhY2UuamF2YQ==) | `73.4104% <ø> (-15.7200%)` | :arrow_down: |
| [...java/org/codehaus/groovy/vmplugin/v8/Selector.java](https://app.codecov.io/gh/apache/groovy/pull/2673?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fvmplugin%2Fv8%2FSelector.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3ZtcGx1Z2luL3Y4L1NlbGVjdG9yLmphdmE=) | `79.0698% <0.0000%> (-1.8826%)` | :arrow_down: |
| [...vmplugin/v8/ColdReflectiveMethodHandleWrapper.java](https://app.codecov.io/gh/apache/groovy/pull/2673?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fvmplugin%2Fv8%2FColdReflectiveMethodHandleWrapper.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3ZtcGx1Z2luL3Y4L0NvbGRSZWZsZWN0aXZlTWV0aG9kSGFuZGxlV3JhcHBlci5qYXZh) | `0.0000% <0.0000%> (ø)` | |
... and [8 files with indirect coverage changes](https://app.codecov.io/gh/apache/groovy/pull/2673/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on code in PR #2673:
URL: https://github.com/apache/groovy/pull/2673#discussion_r3549140460
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/ColdReflectiveMethodHandleWrapper.java:
##########
@@ -0,0 +1,196 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.codehaus.groovy.vmplugin.v8;
+
+import groovy.lang.GroovyObject;
+import groovy.lang.MetaClass;
+import groovy.lang.MetaMethod;
+import org.codehaus.groovy.reflection.CachedMethod;
+import org.codehaus.groovy.runtime.GroovyCategorySupport;
+import org.codehaus.groovy.runtime.wrappers.Wrapper;
+import org.codehaus.groovy.vmplugin.VMPluginFactory;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.SwitchPoint;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+
+/**
+ * Experimental reflective cold-tier wrapper, guarded by the
+ * {@code groovy.indy.cold.reflection} system property.
+ * <p>
+ * The regular cold tier builds a fully guarded MethodHandle chain on the
+ * first invocation of every receiver shape — paying the one-time LambdaForm
+ * creation cost for a MethodType shape determined by the call site's arity
+ * and primitive pattern. This wrapper instead carries only the selected
+ * {@link MetaMethod} plus the data needed to re-validate the selection in
+ * plain Java; invocation happens reflectively via
+ * {@link IndyInterface#invokeColdReflective}. Its bound invocation handle has
+ * the same {@code (Object[])Object} shape for every call site, so the cold
+ * tier spins no per-shape LambdaForms. The full guarded chain is built only
+ * at promotion time (see {@code IndyInterface.fromCacheHandle}), leaving the
+ * hot path unchanged.
+ * <p>
+ * The plain-Java validity checks in {@link #isValidFor} mirror the guards the
+ * full chain would install: switch-point freshness (which also covers
+ * category enter/leave, as those invalidate the switch point), exact argument
+ * classes (at least as strict as the chain's same-class guards), and
+ * metaclass identity for {@code GroovyObject} receivers.
+ */
+class ColdReflectiveMethodHandleWrapper extends MethodHandleWrapper {
+
+ final CacheableCallSite callSite;
+ final Class<?> sender;
+ final String methodName;
+ final int callID;
+ final Boolean safeNavigation;
+ final Boolean thisCall;
+ final Boolean spreadCall;
+ private final MetaClass selectionMetaClass;
+ private final SwitchPoint validity;
+ private final Class<?>[] argClasses;
+ private final MethodHandle reflectiveHandle;
+ /**
+ * Cumulative reflective invocations, independent of the PIC's
+ * consecutive-hit counter (which resets whenever receivers alternate, so
+ * polymorphic sites would otherwise stay on the reflective tier forever).
+ * Racy increments only delay promotion slightly, which is harmless.
+ */
+ private int reflectiveHits;
+
+ private ColdReflectiveMethodHandleWrapper(MetaMethod method, CacheableCallSite callSite, Class<?> sender,
+ String methodName, int callID, Boolean safeNavigation, Boolean thisCall, Boolean spreadCall,
+ MetaClass selectionMetaClass, SwitchPoint validity, Class<?>[] argClasses) {
+ super(null, null, method, true);
+ this.callSite = callSite;
+ this.sender = sender;
+ this.methodName = methodName;
+ this.callID = callID;
+ this.safeNavigation = safeNavigation;
+ this.thisCall = thisCall;
+ this.spreadCall = spreadCall;
+ this.selectionMetaClass = selectionMetaClass;
+ this.validity = validity;
+ this.argClasses = argClasses;
+ this.reflectiveHandle = IndyInterface.COLD_REFLECTIVE_INVOKER.bindTo(this);
+ }
+
+ @Override
+ public MethodHandle getCachedMethodHandle() {
+ return reflectiveHandle;
+ }
+
+ /**
+ * Counts a reflective invocation.
+ *
+ * @return the cumulative reflective invocation count
+ */
+ int incrementReflectiveHits() {
+ return ++reflectiveHits;
+ }
+
+ /**
+ * Plain-Java equivalent of the guards the full handle chain would install.
+ *
+ * @param arguments receiver-first invocation arguments
+ * @return {@code true} if the cached selection is still valid for them
+ */
+ boolean isValidFor(Object[] arguments) {
+ if (validity.hasBeenInvalidated()) return false;
+ Class<?>[] classes = argClasses;
+ if (arguments.length != classes.length) return false;
+ for (int i = 0; i < classes.length; i++) {
+ Object a = arguments[i];
+ if (a == null ? classes[i] != null : a.getClass() != classes[i]) return false;
+ }
+ Object receiver = arguments[0];
+ return !(receiver instanceof GroovyObject go) || go.getMetaClass() == selectionMetaClass;
+ }
+
+ /**
+ * Attempts a reflective cold-tier selection for a plain method call.
+ * "Plain" means: real receiver (non-null, not a {@code Class}, not
+ * {@code GroovyInterceptable}), no spread/safe-null semantics, cacheable
+ * selection resolving to a public, non-static, non-category
+ * {@link CachedMethod} on a public class that is not caller-sensitive.
+ * Anything else returns {@code null} and takes the full handle path.
+ *
+ * @return the reflective wrapper, or {@code null} if unsupported
+ */
+ static MethodHandleWrapper tryBuild(Selector selector, CacheableCallSite callSite, Class<?> sender,
+ String methodName, int callID, Boolean safeNavigation, Boolean thisCall, Boolean spreadCall,
+ Object[] arguments) {
+ MetaMethod selected = selector.selectForColdReflection();
+ if (selected == null || selected instanceof GroovyCategorySupport.CategoryMethod) return null;
+ MetaClass mc = selector.getSelectionMetaClass();
+ if (mc == null || !selector.cache) return null;
+ MetaMethod transformed = VMPluginFactory.getPlugin().transformMetaMethod(mc, selected, sender);
+ if (!(transformed instanceof CachedMethod cm)) return null;
+ int modifiers = cm.getModifiers();
+ if (Modifier.isStatic(modifiers) || !Modifier.isPublic(modifiers)) return null;
+ if (!Modifier.isPublic(cm.getDeclaringClass().getModifiers())) return null;
+ if (isCallerSensitive(cm, arguments[0].getClass())) return null;
+ Class<?>[] argClasses = new Class[arguments.length];
+ for (int i = 0; i < arguments.length; i++) {
+ Object a = arguments[i];
+ if (a instanceof Wrapper) return null; // cast markers need the full coercion path
+ argClasses[i] = (a == null) ? null : a.getClass();
+ }
+ return new ColdReflectiveMethodHandleWrapper(cm, callSite, sender, methodName, callID,
+ safeNavigation, thisCall, spreadCall, mc, IndyInterface.switchPoint, argClasses);
+ }
+
+ /**
+ * Reflective invocation would report the wrong caller for caller-sensitive
+ * methods (and promotion would then change the observed caller mid-run),
+ * so such targets stay on the full handle path. The per-method annotation
+ * probe lives on {@link CachedMethod#isCallerSensitive()} so its cost is
+ * paid once per method rather than per call site. Two extensions are
+ * handled here: some JDK methods are caller-context sensitive via stack
+ * walking without carrying the annotation — notably object serialization,
+ * whose {@code latestUserDefinedLoader} would see the reflective frames
+ * instead of the calling script's class loader — and the metaclass may
+ * select an <em>interface</em> method (e.g. {@code ObjectInput.readObject()})
+ * whose implementation is the sensitive one, so abstract selections are
+ * probed against the runtime receiver's implementation. When a probe
+ * cannot decide, the method is treated as sensitive.
+ */
+ private static boolean isCallerSensitive(CachedMethod cm, Class<?> receiverClass) {
+ Class<?> declaringClass = cm.getDeclaringClass().getTheClass();
+ if (java.io.ObjectInput.class.isAssignableFrom(declaringClass)
+ || java.io.ObjectOutput.class.isAssignableFrom(declaringClass)
+ || java.io.ObjectInputStream.class.isAssignableFrom(declaringClass)
+ || java.io.ObjectOutputStream.class.isAssignableFrom(declaringClass)) {
+ return true;
+ }
+ if (cm.isCallerSensitive()) return true;
+ Method method = cm.getCachedMethod();
+ if (declaringClass.isInterface() || Modifier.isAbstract(method.getModifiers())) {
+ // virtual dispatch may land on a caller-sensitive implementation
+ try {
+ Method implementation = receiverClass.getMethod(method.getName(), method.getParameterTypes());
+ CachedMethod implCached = CachedMethod.find(implementation);
+ if (implCached != null) return implCached.isCallerSensitive();
+ } catch (Throwable ignore) {
+ }
+ return true; // could not resolve the implementation: stay safe
+ }
+ return false;
Review Comment:
(isCallerSensitive): Restructured as you suggest: everything method-determined — the annotation probe and the declaring-class cases like serialization — is now behind cm.isCallerSensitive(), cached per method. And yes, abstract selections really occur: the metaclass picks ObjectInput.readObject() for a call on an ObjectInputStream (that exact case was found by a failing serialization test), and the sensitivity then belongs to the receiver's implementation, which is the one receiver-dependent check left in the wrapper. Good catch on defaults: the old isInterface() clause could conservatively mark a default method when implementation lookup failed — now non-abstract interface methods are probed directly, and a default method verifiably takes the reflective tier.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on code in PR #2673:
URL: https://github.com/apache/groovy/pull/2673#discussion_r3536510714
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/ColdReflectiveMethodHandleWrapper.java:
##########
@@ -0,0 +1,203 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.codehaus.groovy.vmplugin.v8;
+
+import groovy.lang.GroovyObject;
+import groovy.lang.MetaClass;
+import groovy.lang.MetaMethod;
+import org.codehaus.groovy.reflection.CachedMethod;
+import org.codehaus.groovy.runtime.GroovyCategorySupport;
+import org.codehaus.groovy.runtime.wrappers.Wrapper;
+import org.codehaus.groovy.vmplugin.VMPluginFactory;
+
+import java.lang.annotation.Annotation;
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.SwitchPoint;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+
+/**
+ * Experimental reflective cold-tier wrapper, guarded by the
+ * {@code groovy.indy.cold.reflection} system property.
+ * <p>
+ * The regular cold tier builds a fully guarded MethodHandle chain on the
+ * first invocation of every receiver shape — paying the one-time LambdaForm
+ * creation cost for a MethodType shape determined by the call site's arity
+ * and primitive pattern. This wrapper instead carries only the selected
+ * {@link MetaMethod} plus the data needed to re-validate the selection in
+ * plain Java; invocation happens reflectively via
+ * {@link IndyInterface#invokeColdReflective}. Its bound invocation handle has
+ * the same {@code (Object[])Object} shape for every call site, so the cold
+ * tier spins no per-shape LambdaForms. The full guarded chain is built only
+ * at promotion time (see {@code IndyInterface.fromCacheHandle}), leaving the
+ * hot path unchanged.
+ * <p>
+ * The plain-Java validity checks in {@link #isValidFor} mirror the guards the
+ * full chain would install: switch-point freshness (which also covers
+ * category enter/leave, as those invalidate the switch point), exact argument
+ * classes (at least as strict as the chain's same-class guards), and
+ * metaclass identity for {@code GroovyObject} receivers.
+ */
+class ColdReflectiveMethodHandleWrapper extends MethodHandleWrapper {
+
+ final CacheableCallSite callSite;
+ final Class<?> sender;
+ final String methodName;
+ final int callID;
+ final Boolean safeNavigation;
+ final Boolean thisCall;
+ final Boolean spreadCall;
+ private final MetaClass selectionMetaClass;
+ private final SwitchPoint validity;
+ private final Class<?>[] argClasses;
+ private final MethodHandle reflectiveHandle;
+ /**
+ * Cumulative reflective invocations, independent of the PIC's
+ * consecutive-hit counter (which resets whenever receivers alternate, so
+ * polymorphic sites would otherwise stay on the reflective tier forever).
+ * Racy increments only delay promotion slightly, which is harmless.
+ */
+ private int reflectiveHits;
+
+ private ColdReflectiveMethodHandleWrapper(MetaMethod method, CacheableCallSite callSite, Class<?> sender,
+ String methodName, int callID, Boolean safeNavigation, Boolean thisCall, Boolean spreadCall,
+ MetaClass selectionMetaClass, SwitchPoint validity, Class<?>[] argClasses) {
+ super(null, null, method, true);
+ this.callSite = callSite;
+ this.sender = sender;
+ this.methodName = methodName;
+ this.callID = callID;
+ this.safeNavigation = safeNavigation;
+ this.thisCall = thisCall;
+ this.spreadCall = spreadCall;
+ this.selectionMetaClass = selectionMetaClass;
+ this.validity = validity;
+ this.argClasses = argClasses;
+ this.reflectiveHandle = IndyInterface.COLD_REFLECTIVE_INVOKER.bindTo(this);
+ }
+
+ @Override
+ public MethodHandle getCachedMethodHandle() {
+ return reflectiveHandle;
+ }
+
+ /**
+ * Counts a reflective invocation.
+ *
+ * @return the cumulative reflective invocation count
+ */
+ int incrementReflectiveHits() {
+ return ++reflectiveHits;
+ }
+
+ /**
+ * Plain-Java equivalent of the guards the full handle chain would install.
+ *
+ * @param arguments receiver-first invocation arguments
+ * @return {@code true} if the cached selection is still valid for them
+ */
+ boolean isValidFor(Object[] arguments) {
+ if (validity.hasBeenInvalidated()) return false;
+ Class<?>[] classes = argClasses;
+ if (arguments.length != classes.length) return false;
+ for (int i = 0; i < classes.length; i++) {
+ Object a = arguments[i];
+ if (a == null ? classes[i] != null : a.getClass() != classes[i]) return false;
+ }
+ Object receiver = arguments[0];
+ return !(receiver instanceof GroovyObject go) || go.getMetaClass() == selectionMetaClass;
+ }
+
+ /**
+ * Attempts a reflective cold-tier selection for a plain method call.
+ * "Plain" means: real receiver (non-null, not a {@code Class}, not
+ * {@code GroovyInterceptable}), no spread/safe-null semantics, cacheable
+ * selection resolving to a public, non-static, non-category
+ * {@link CachedMethod} on a public class that is not caller-sensitive.
+ * Anything else returns {@code null} and takes the full handle path.
+ *
+ * @return the reflective wrapper, or {@code null} if unsupported
+ */
+ static MethodHandleWrapper tryBuild(Selector selector, CacheableCallSite callSite, Class<?> sender,
+ String methodName, int callID, Boolean safeNavigation, Boolean thisCall, Boolean spreadCall,
+ Object[] arguments) {
+ MetaMethod selected = selector.selectForColdReflection();
+ if (selected == null || selected instanceof GroovyCategorySupport.CategoryMethod) return null;
+ MetaClass mc = selector.getSelectionMetaClass();
+ if (mc == null || !selector.cache) return null;
+ MetaMethod transformed = VMPluginFactory.getPlugin().transformMetaMethod(mc, selected, sender);
+ if (!(transformed instanceof CachedMethod cm)) return null;
+ int modifiers = cm.getModifiers();
+ if (Modifier.isStatic(modifiers) || !Modifier.isPublic(modifiers)) return null;
+ if (!Modifier.isPublic(cm.getDeclaringClass().getModifiers())) return null;
+ if (isCallerSensitive(cm.getCachedMethod(), arguments[0].getClass())) return null;
+ Class<?>[] argClasses = new Class[arguments.length];
+ for (int i = 0; i < arguments.length; i++) {
+ Object a = arguments[i];
+ if (a instanceof Wrapper) return null; // cast markers need the full coercion path
+ argClasses[i] = (a == null) ? null : a.getClass();
+ }
+ return new ColdReflectiveMethodHandleWrapper(cm, callSite, sender, methodName, callID,
+ safeNavigation, thisCall, spreadCall, mc, IndyInterface.switchPoint, argClasses);
+ }
+
+ /**
+ * Reflective invocation would report the wrong caller for caller-sensitive
+ * methods (and promotion would then change the observed caller mid-run),
+ * so such targets stay on the full handle path. Beyond the
+ * {@code @CallerSensitive} annotation, some JDK methods are caller-context
+ * sensitive via stack walking without carrying the annotation — notably
+ * object serialization, whose {@code latestUserDefinedLoader} would see
+ * the reflective frames instead of the calling script's class loader.
+ * Note that the metaclass may select an <em>interface</em> method (e.g.
+ * {@code ObjectInput.readObject()}) whose implementation is the sensitive
+ * one, so abstract selections are probed against the runtime receiver's
+ * implementation. When the probe cannot decide, the method is treated as
+ * sensitive.
+ */
+ private static boolean isCallerSensitive(Method method, Class<?> receiverClass) {
+ Class<?> declaringClass = method.getDeclaringClass();
+ if (java.io.ObjectInput.class.isAssignableFrom(declaringClass)
+ || java.io.ObjectOutput.class.isAssignableFrom(declaringClass)
+ || java.io.ObjectInputStream.class.isAssignableFrom(declaringClass)
+ || java.io.ObjectOutputStream.class.isAssignableFrom(declaringClass)) {
+ return true;
+ }
+ try {
+ if (hasCallerSensitiveAnnotation(method)) return true;
+ if (declaringClass.isInterface() || Modifier.isAbstract(method.getModifiers())) {
+ // virtual dispatch may land on a caller-sensitive implementation
+ Method implementation = receiverClass.getMethod(method.getName(), method.getParameterTypes());
+ return hasCallerSensitiveAnnotation(implementation);
+ }
+ return false;
+ } catch (Throwable ignore) {
+ return true;
+ }
Review Comment:
changed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on PR #2673:
URL: https://github.com/apache/groovy/pull/2673#issuecomment-4910894521
@blackdrag updated - please reassess
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "blackdrag (via GitHub)" <gi...@apache.org>.
blackdrag commented on code in PR #2673:
URL: https://github.com/apache/groovy/pull/2673#discussion_r3539504587
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
@@ -1226,6 +1251,26 @@ void buildInvokeHandle() {
addExceptionHandler();
}
}
+
+ @Override
+ MetaMethod selectForColdReflection() {
+ if (safeNavigation || spread || args[0] == null
+ || args[0] instanceof Class || args[0] instanceof GroovyInterceptable) {
+ return null;
+ }
Review Comment:
sorry, I think I was too fast and mixed up things. The potential method call is not asType, but asBoolean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #2673:
URL: https://github.com/apache/groovy/pull/2673#issuecomment-4911102342
## ✅ All tests passed ✅
🏷️ Commit: fec99f71755c7df79884f6548e35ad7326a54b23
▶️ Tests: 36147 executed
⚪️ Checks: 31/31 completed
---
_Learn more about TestLens at [testlens.app](https://testlens.app)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #2673:
URL: https://github.com/apache/groovy/pull/2673#issuecomment-4946858645
## ✅ All tests passed ✅
🏷️ Commit: 91aa7b466cf16fed14b8819b0991dd555d01dc36
▶️ Tests: 103178 executed
⚪️ Checks: 31/31 completed
---
_Learn more about TestLens at [testlens.app](https://testlens.app)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #2673:
URL: https://github.com/apache/groovy/pull/2673#issuecomment-4904401134
## ✅ All tests passed ✅
🏷️ Commit: d28057833f16aa41d9f03982199f8f1242f97a16
▶️ Tests: 102853 executed
⚪️ Checks: 31/31 completed
---
_Learn more about TestLens at [testlens.app](https://testlens.app)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #2673:
URL: https://github.com/apache/groovy/pull/2673#issuecomment-4921774185
## ✅ All tests passed ✅
🏷️ Commit: 750878ef1e20b9a2ab7b65ec6c6b71c9cb588de0
▶️ Tests: 103231 executed
⚪️ Checks: 31/31 completed
---
_Learn more about TestLens at [testlens.app](https://testlens.app)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on code in PR #2673:
URL: https://github.com/apache/groovy/pull/2673#discussion_r3549119844
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
Review Comment:
(binary file): The head side is already clean — the NUL lives in the base blob on master (a literal \0 embedded in the GEP-15 COMPOUND_ASSIGN javadoc to illustrate NAME_SEPARATOR), so git treats the base as binary and no diff can render. Pushed the one-line javadoc fix to master separately; after the next rebase this file will diff normally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #2673:
URL: https://github.com/apache/groovy/pull/2673#issuecomment-4903268052
## ✅ All tests passed ✅
🏷️ Commit: 8b41cd8df68e9e569771fe70c37b8dec9cdf8826
▶️ Tests: 103201 executed
⚪️ Checks: 31/31 completed
---
_Learn more about TestLens at [testlens.app](https://testlens.app)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "blackdrag (via GitHub)" <gi...@apache.org>.
blackdrag commented on code in PR #2673:
URL: https://github.com/apache/groovy/pull/2673#discussion_r3536181823
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
Review Comment:
this is marked as binary file somehow?
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/ColdReflectiveMethodHandleWrapper.java:
##########
@@ -0,0 +1,203 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.codehaus.groovy.vmplugin.v8;
+
+import groovy.lang.GroovyObject;
+import groovy.lang.MetaClass;
+import groovy.lang.MetaMethod;
+import org.codehaus.groovy.reflection.CachedMethod;
+import org.codehaus.groovy.runtime.GroovyCategorySupport;
+import org.codehaus.groovy.runtime.wrappers.Wrapper;
+import org.codehaus.groovy.vmplugin.VMPluginFactory;
+
+import java.lang.annotation.Annotation;
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.SwitchPoint;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+
+/**
+ * Experimental reflective cold-tier wrapper, guarded by the
+ * {@code groovy.indy.cold.reflection} system property.
+ * <p>
+ * The regular cold tier builds a fully guarded MethodHandle chain on the
+ * first invocation of every receiver shape — paying the one-time LambdaForm
+ * creation cost for a MethodType shape determined by the call site's arity
+ * and primitive pattern. This wrapper instead carries only the selected
+ * {@link MetaMethod} plus the data needed to re-validate the selection in
+ * plain Java; invocation happens reflectively via
+ * {@link IndyInterface#invokeColdReflective}. Its bound invocation handle has
+ * the same {@code (Object[])Object} shape for every call site, so the cold
+ * tier spins no per-shape LambdaForms. The full guarded chain is built only
+ * at promotion time (see {@code IndyInterface.fromCacheHandle}), leaving the
+ * hot path unchanged.
+ * <p>
+ * The plain-Java validity checks in {@link #isValidFor} mirror the guards the
+ * full chain would install: switch-point freshness (which also covers
+ * category enter/leave, as those invalidate the switch point), exact argument
+ * classes (at least as strict as the chain's same-class guards), and
+ * metaclass identity for {@code GroovyObject} receivers.
+ */
+class ColdReflectiveMethodHandleWrapper extends MethodHandleWrapper {
+
+ final CacheableCallSite callSite;
+ final Class<?> sender;
+ final String methodName;
+ final int callID;
+ final Boolean safeNavigation;
+ final Boolean thisCall;
+ final Boolean spreadCall;
+ private final MetaClass selectionMetaClass;
+ private final SwitchPoint validity;
+ private final Class<?>[] argClasses;
+ private final MethodHandle reflectiveHandle;
+ /**
+ * Cumulative reflective invocations, independent of the PIC's
+ * consecutive-hit counter (which resets whenever receivers alternate, so
+ * polymorphic sites would otherwise stay on the reflective tier forever).
+ * Racy increments only delay promotion slightly, which is harmless.
+ */
+ private int reflectiveHits;
+
+ private ColdReflectiveMethodHandleWrapper(MetaMethod method, CacheableCallSite callSite, Class<?> sender,
+ String methodName, int callID, Boolean safeNavigation, Boolean thisCall, Boolean spreadCall,
+ MetaClass selectionMetaClass, SwitchPoint validity, Class<?>[] argClasses) {
+ super(null, null, method, true);
+ this.callSite = callSite;
+ this.sender = sender;
+ this.methodName = methodName;
+ this.callID = callID;
+ this.safeNavigation = safeNavigation;
+ this.thisCall = thisCall;
+ this.spreadCall = spreadCall;
+ this.selectionMetaClass = selectionMetaClass;
+ this.validity = validity;
+ this.argClasses = argClasses;
+ this.reflectiveHandle = IndyInterface.COLD_REFLECTIVE_INVOKER.bindTo(this);
+ }
+
+ @Override
+ public MethodHandle getCachedMethodHandle() {
+ return reflectiveHandle;
+ }
+
+ /**
+ * Counts a reflective invocation.
+ *
+ * @return the cumulative reflective invocation count
+ */
+ int incrementReflectiveHits() {
+ return ++reflectiveHits;
+ }
+
+ /**
+ * Plain-Java equivalent of the guards the full handle chain would install.
+ *
+ * @param arguments receiver-first invocation arguments
+ * @return {@code true} if the cached selection is still valid for them
+ */
+ boolean isValidFor(Object[] arguments) {
+ if (validity.hasBeenInvalidated()) return false;
+ Class<?>[] classes = argClasses;
+ if (arguments.length != classes.length) return false;
+ for (int i = 0; i < classes.length; i++) {
+ Object a = arguments[i];
+ if (a == null ? classes[i] != null : a.getClass() != classes[i]) return false;
+ }
+ Object receiver = arguments[0];
+ return !(receiver instanceof GroovyObject go) || go.getMetaClass() == selectionMetaClass;
+ }
+
+ /**
+ * Attempts a reflective cold-tier selection for a plain method call.
+ * "Plain" means: real receiver (non-null, not a {@code Class}, not
+ * {@code GroovyInterceptable}), no spread/safe-null semantics, cacheable
+ * selection resolving to a public, non-static, non-category
+ * {@link CachedMethod} on a public class that is not caller-sensitive.
+ * Anything else returns {@code null} and takes the full handle path.
+ *
+ * @return the reflective wrapper, or {@code null} if unsupported
+ */
+ static MethodHandleWrapper tryBuild(Selector selector, CacheableCallSite callSite, Class<?> sender,
+ String methodName, int callID, Boolean safeNavigation, Boolean thisCall, Boolean spreadCall,
+ Object[] arguments) {
+ MetaMethod selected = selector.selectForColdReflection();
+ if (selected == null || selected instanceof GroovyCategorySupport.CategoryMethod) return null;
+ MetaClass mc = selector.getSelectionMetaClass();
+ if (mc == null || !selector.cache) return null;
+ MetaMethod transformed = VMPluginFactory.getPlugin().transformMetaMethod(mc, selected, sender);
+ if (!(transformed instanceof CachedMethod cm)) return null;
+ int modifiers = cm.getModifiers();
+ if (Modifier.isStatic(modifiers) || !Modifier.isPublic(modifiers)) return null;
+ if (!Modifier.isPublic(cm.getDeclaringClass().getModifiers())) return null;
+ if (isCallerSensitive(cm.getCachedMethod(), arguments[0].getClass())) return null;
+ Class<?>[] argClasses = new Class[arguments.length];
+ for (int i = 0; i < arguments.length; i++) {
+ Object a = arguments[i];
+ if (a instanceof Wrapper) return null; // cast markers need the full coercion path
+ argClasses[i] = (a == null) ? null : a.getClass();
+ }
+ return new ColdReflectiveMethodHandleWrapper(cm, callSite, sender, methodName, callID,
+ safeNavigation, thisCall, spreadCall, mc, IndyInterface.switchPoint, argClasses);
+ }
+
+ /**
+ * Reflective invocation would report the wrong caller for caller-sensitive
+ * methods (and promotion would then change the observed caller mid-run),
+ * so such targets stay on the full handle path. Beyond the
+ * {@code @CallerSensitive} annotation, some JDK methods are caller-context
+ * sensitive via stack walking without carrying the annotation — notably
+ * object serialization, whose {@code latestUserDefinedLoader} would see
+ * the reflective frames instead of the calling script's class loader.
+ * Note that the metaclass may select an <em>interface</em> method (e.g.
+ * {@code ObjectInput.readObject()}) whose implementation is the sensitive
+ * one, so abstract selections are probed against the runtime receiver's
+ * implementation. When the probe cannot decide, the method is treated as
+ * sensitive.
+ */
+ private static boolean isCallerSensitive(Method method, Class<?> receiverClass) {
+ Class<?> declaringClass = method.getDeclaringClass();
+ if (java.io.ObjectInput.class.isAssignableFrom(declaringClass)
+ || java.io.ObjectOutput.class.isAssignableFrom(declaringClass)
+ || java.io.ObjectInputStream.class.isAssignableFrom(declaringClass)
+ || java.io.ObjectOutputStream.class.isAssignableFrom(declaringClass)) {
+ return true;
+ }
+ try {
+ if (hasCallerSensitiveAnnotation(method)) return true;
+ if (declaringClass.isInterface() || Modifier.isAbstract(method.getModifiers())) {
+ // virtual dispatch may land on a caller-sensitive implementation
+ Method implementation = receiverClass.getMethod(method.getName(), method.getParameterTypes());
+ return hasCallerSensitiveAnnotation(implementation);
+ }
+ return false;
+ } catch (Throwable ignore) {
+ return true;
+ }
Review Comment:
I think this, if we keep it, is an information that should exist on Cached method and reused there to not to have to pay the information on a per callsite base
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
@@ -1226,6 +1251,26 @@ void buildInvokeHandle() {
addExceptionHandler();
}
}
+
+ @Override
+ MetaMethod selectForColdReflection() {
+ if (safeNavigation || spread || args[0] == null
+ || args[0] instanceof Class || args[0] instanceof GroovyInterceptable) {
+ return null;
+ }
Review Comment:
spread and safeNavidation should be always false for CastSelector. Checking for Class looks strange here. Why not look for a asType method on Class added by the metaclass? Also why is null excluded?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on code in PR #2673:
URL: https://github.com/apache/groovy/pull/2673#discussion_r3536515856
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
@@ -1226,6 +1251,26 @@ void buildInvokeHandle() {
addExceptionHandler();
}
}
+
+ @Override
+ MetaMethod selectForColdReflection() {
+ if (safeNavigation || spread || args[0] == null
+ || args[0] instanceof Class || args[0] instanceof GroovyInterceptable) {
+ return null;
+ }
Review Comment:
AI read: That hook is only reached for CallType.METHOD — the fallback gates on it, so CastSelector/InterfaceSelector are never asked; I've added a defensive check plus comments since that wasn't visible from the diff. The instanceof Class test isn't about asType/cast: a Class receiver means a static-method-style call, and those promote on their first hit anyway (GROOVY-11935), so the reflective tier would buy nothing. Null receivers mean NullObject MOP dispatch, which I kept out of the plain-call scope for the spike — supportable later if the numbers justify it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #2673:
URL: https://github.com/apache/groovy/pull/2673#issuecomment-4910080156
## ✅ All tests passed ✅
🏷️ Commit: e1d35a3613284680391dd2ef7085d8a45a246b5b
▶️ Tests: 103201 executed
⚪️ Checks: 31/31 completed
---
_Learn more about TestLens at [testlens.app](https://testlens.app)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on PR #2673:
URL: https://github.com/apache/groovy/pull/2673#issuecomment-4921909263
@blackdrag Updated - ready for another assessment if you have time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on code in PR #2673:
URL: https://github.com/apache/groovy/pull/2673#discussion_r3549143055
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
@@ -1226,6 +1251,35 @@ void buildInvokeHandle() {
addExceptionHandler();
}
}
+
+ @Override
+ MetaMethod selectForColdReflection() {
+ // defensive: the cold tier is only for plain method calls; the
+ // fallback gates on CallType.METHOD, so subclasses (CastSelector,
+ // InterfaceSelector) are never asked, but make it explicit here
+ if (callType != CallType.METHOD) return null;
+ // these all need the full handle path: spread and safe-null have
+ // their own adaptation/constant handles; a null receiver means
+ // NullObject MOP dispatch; a Class receiver is a static-method
Review Comment:
(NullObject): You're right — nothing fundamental prevents serving NullObject's methods reflectively; the exclusion was spike conservatism. Marked as TODO as suggested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on code in PR #2673:
URL: https://github.com/apache/groovy/pull/2673#discussion_r3536507025
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
Review Comment:
there was a \0000 char in the file - fixed now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "blackdrag (via GitHub)" <gi...@apache.org>.
blackdrag commented on code in PR #2673:
URL: https://github.com/apache/groovy/pull/2673#discussion_r3538044994
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
@@ -1226,6 +1251,26 @@ void buildInvokeHandle() {
addExceptionHandler();
}
}
+
+ @Override
+ MetaMethod selectForColdReflection() {
+ if (safeNavigation || spread || args[0] == null
+ || args[0] instanceof Class || args[0] instanceof GroovyInterceptable) {
+ return null;
+ }
Review Comment:
CastSelector is not supposed to be used for CallType.METHOD, also there are no static method calls for CastSelector. CastSelector is for non-trivial casts that may result in asType calls. if the receiver is Class it means we are dealing with something like "(T) c" where c is an instance of Class. It is not for a normal method call to asType(Class). Things left out of the spike are ok, if the PR is not intended for merge. Otherwise it should have at the very least a TODO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "blackdrag (via GitHub)" <gi...@apache.org>.
blackdrag commented on code in PR #2673:
URL: https://github.com/apache/groovy/pull/2673#discussion_r3547645252
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/ColdReflectiveMethodHandleWrapper.java:
##########
@@ -0,0 +1,196 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.codehaus.groovy.vmplugin.v8;
+
+import groovy.lang.GroovyObject;
+import groovy.lang.MetaClass;
+import groovy.lang.MetaMethod;
+import org.codehaus.groovy.reflection.CachedMethod;
+import org.codehaus.groovy.runtime.GroovyCategorySupport;
+import org.codehaus.groovy.runtime.wrappers.Wrapper;
+import org.codehaus.groovy.vmplugin.VMPluginFactory;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.SwitchPoint;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+
+/**
+ * Experimental reflective cold-tier wrapper, guarded by the
+ * {@code groovy.indy.cold.reflection} system property.
+ * <p>
+ * The regular cold tier builds a fully guarded MethodHandle chain on the
+ * first invocation of every receiver shape — paying the one-time LambdaForm
+ * creation cost for a MethodType shape determined by the call site's arity
+ * and primitive pattern. This wrapper instead carries only the selected
+ * {@link MetaMethod} plus the data needed to re-validate the selection in
+ * plain Java; invocation happens reflectively via
+ * {@link IndyInterface#invokeColdReflective}. Its bound invocation handle has
+ * the same {@code (Object[])Object} shape for every call site, so the cold
+ * tier spins no per-shape LambdaForms. The full guarded chain is built only
+ * at promotion time (see {@code IndyInterface.fromCacheHandle}), leaving the
+ * hot path unchanged.
+ * <p>
+ * The plain-Java validity checks in {@link #isValidFor} mirror the guards the
+ * full chain would install: switch-point freshness (which also covers
+ * category enter/leave, as those invalidate the switch point), exact argument
+ * classes (at least as strict as the chain's same-class guards), and
+ * metaclass identity for {@code GroovyObject} receivers.
+ */
+class ColdReflectiveMethodHandleWrapper extends MethodHandleWrapper {
+
+ final CacheableCallSite callSite;
+ final Class<?> sender;
+ final String methodName;
+ final int callID;
+ final Boolean safeNavigation;
+ final Boolean thisCall;
+ final Boolean spreadCall;
+ private final MetaClass selectionMetaClass;
+ private final SwitchPoint validity;
+ private final Class<?>[] argClasses;
+ private final MethodHandle reflectiveHandle;
+ /**
+ * Cumulative reflective invocations, independent of the PIC's
+ * consecutive-hit counter (which resets whenever receivers alternate, so
+ * polymorphic sites would otherwise stay on the reflective tier forever).
+ * Racy increments only delay promotion slightly, which is harmless.
+ */
+ private int reflectiveHits;
+
+ private ColdReflectiveMethodHandleWrapper(MetaMethod method, CacheableCallSite callSite, Class<?> sender,
+ String methodName, int callID, Boolean safeNavigation, Boolean thisCall, Boolean spreadCall,
+ MetaClass selectionMetaClass, SwitchPoint validity, Class<?>[] argClasses) {
+ super(null, null, method, true);
+ this.callSite = callSite;
+ this.sender = sender;
+ this.methodName = methodName;
+ this.callID = callID;
+ this.safeNavigation = safeNavigation;
+ this.thisCall = thisCall;
+ this.spreadCall = spreadCall;
+ this.selectionMetaClass = selectionMetaClass;
+ this.validity = validity;
+ this.argClasses = argClasses;
+ this.reflectiveHandle = IndyInterface.COLD_REFLECTIVE_INVOKER.bindTo(this);
+ }
+
+ @Override
+ public MethodHandle getCachedMethodHandle() {
+ return reflectiveHandle;
+ }
+
+ /**
+ * Counts a reflective invocation.
+ *
+ * @return the cumulative reflective invocation count
+ */
+ int incrementReflectiveHits() {
+ return ++reflectiveHits;
+ }
+
+ /**
+ * Plain-Java equivalent of the guards the full handle chain would install.
+ *
+ * @param arguments receiver-first invocation arguments
+ * @return {@code true} if the cached selection is still valid for them
+ */
+ boolean isValidFor(Object[] arguments) {
+ if (validity.hasBeenInvalidated()) return false;
+ Class<?>[] classes = argClasses;
+ if (arguments.length != classes.length) return false;
+ for (int i = 0; i < classes.length; i++) {
+ Object a = arguments[i];
+ if (a == null ? classes[i] != null : a.getClass() != classes[i]) return false;
+ }
+ Object receiver = arguments[0];
+ return !(receiver instanceof GroovyObject go) || go.getMetaClass() == selectionMetaClass;
+ }
+
+ /**
+ * Attempts a reflective cold-tier selection for a plain method call.
+ * "Plain" means: real receiver (non-null, not a {@code Class}, not
+ * {@code GroovyInterceptable}), no spread/safe-null semantics, cacheable
+ * selection resolving to a public, non-static, non-category
+ * {@link CachedMethod} on a public class that is not caller-sensitive.
+ * Anything else returns {@code null} and takes the full handle path.
+ *
+ * @return the reflective wrapper, or {@code null} if unsupported
+ */
+ static MethodHandleWrapper tryBuild(Selector selector, CacheableCallSite callSite, Class<?> sender,
+ String methodName, int callID, Boolean safeNavigation, Boolean thisCall, Boolean spreadCall,
+ Object[] arguments) {
+ MetaMethod selected = selector.selectForColdReflection();
+ if (selected == null || selected instanceof GroovyCategorySupport.CategoryMethod) return null;
+ MetaClass mc = selector.getSelectionMetaClass();
+ if (mc == null || !selector.cache) return null;
+ MetaMethod transformed = VMPluginFactory.getPlugin().transformMetaMethod(mc, selected, sender);
+ if (!(transformed instanceof CachedMethod cm)) return null;
+ int modifiers = cm.getModifiers();
+ if (Modifier.isStatic(modifiers) || !Modifier.isPublic(modifiers)) return null;
+ if (!Modifier.isPublic(cm.getDeclaringClass().getModifiers())) return null;
+ if (isCallerSensitive(cm, arguments[0].getClass())) return null;
+ Class<?>[] argClasses = new Class[arguments.length];
+ for (int i = 0; i < arguments.length; i++) {
+ Object a = arguments[i];
+ if (a instanceof Wrapper) return null; // cast markers need the full coercion path
+ argClasses[i] = (a == null) ? null : a.getClass();
+ }
+ return new ColdReflectiveMethodHandleWrapper(cm, callSite, sender, methodName, callID,
+ safeNavigation, thisCall, spreadCall, mc, IndyInterface.switchPoint, argClasses);
+ }
+
+ /**
+ * Reflective invocation would report the wrong caller for caller-sensitive
+ * methods (and promotion would then change the observed caller mid-run),
+ * so such targets stay on the full handle path. The per-method annotation
+ * probe lives on {@link CachedMethod#isCallerSensitive()} so its cost is
+ * paid once per method rather than per call site. Two extensions are
+ * handled here: some JDK methods are caller-context sensitive via stack
+ * walking without carrying the annotation — notably object serialization,
+ * whose {@code latestUserDefinedLoader} would see the reflective frames
+ * instead of the calling script's class loader — and the metaclass may
+ * select an <em>interface</em> method (e.g. {@code ObjectInput.readObject()})
+ * whose implementation is the sensitive one, so abstract selections are
+ * probed against the runtime receiver's implementation. When a probe
+ * cannot decide, the method is treated as sensitive.
+ */
+ private static boolean isCallerSensitive(CachedMethod cm, Class<?> receiverClass) {
+ Class<?> declaringClass = cm.getDeclaringClass().getTheClass();
+ if (java.io.ObjectInput.class.isAssignableFrom(declaringClass)
+ || java.io.ObjectOutput.class.isAssignableFrom(declaringClass)
+ || java.io.ObjectInputStream.class.isAssignableFrom(declaringClass)
+ || java.io.ObjectOutputStream.class.isAssignableFrom(declaringClass)) {
+ return true;
+ }
+ if (cm.isCallerSensitive()) return true;
+ Method method = cm.getCachedMethod();
+ if (declaringClass.isInterface() || Modifier.isAbstract(method.getModifiers())) {
+ // virtual dispatch may land on a caller-sensitive implementation
+ try {
+ Method implementation = receiverClass.getMethod(method.getName(), method.getParameterTypes());
+ CachedMethod implCached = CachedMethod.find(implementation);
+ if (implCached != null) return implCached.isCallerSensitive();
+ } catch (Throwable ignore) {
+ }
+ return true; // could not resolve the implementation: stay safe
+ }
+ return false;
Review Comment:
I don't understand why we are doing this like this. Why can all this information not be behind cm.isCallerSensitive()? At least the part that depends on the declaring class. Then... the interface and abstract method case. We really end up with those? furthermore I think this does not handle default methods, well I should say it may marks them as callsersitive even if they are not.
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
@@ -1226,6 +1251,35 @@ void buildInvokeHandle() {
addExceptionHandler();
}
}
+
+ @Override
+ MetaMethod selectForColdReflection() {
+ // defensive: the cold tier is only for plain method calls; the
+ // fallback gates on CallType.METHOD, so subclasses (CastSelector,
+ // InterfaceSelector) are never asked, but make it explicit here
+ if (callType != CallType.METHOD) return null;
Review Comment:
I would prefer a GroovyBugException. Like this it could hide a problem introduced by a change.
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
@@ -1226,6 +1251,35 @@ void buildInvokeHandle() {
addExceptionHandler();
}
}
+
+ @Override
+ MetaMethod selectForColdReflection() {
+ // defensive: the cold tier is only for plain method calls; the
+ // fallback gates on CallType.METHOD, so subclasses (CastSelector,
+ // InterfaceSelector) are never asked, but make it explicit here
+ if (callType != CallType.METHOD) return null;
+ // these all need the full handle path: spread and safe-null have
+ // their own adaptation/constant handles; a null receiver means
+ // NullObject MOP dispatch; a Class receiver is a static-method
Review Comment:
why do we need to dispatch through the normal MOP for NullObject? NullObject has methods like any other class, only the receiver is null. Maybe it does not have to be part of this PR, but I would mark its investigation as TODO
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
Review Comment:
There is still no diff on the file and it is marked as binary
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12137: experimental reflective cold tier for indy dispatch (sp… [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert merged PR #2673:
URL: https://github.com/apache/groovy/pull/2673
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org