[
https://issues.apache.org/jira/browse/GROOVY-12191?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18099297#comment-18099297
]
ASF GitHub Bot commented on GROOVY-12191:
-----------------------------------------
blackdrag commented on code in PR #2736:
URL: https://github.com/apache/groovy/pull/2736#discussion_r3653158867
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
@@ -215,29 +216,86 @@ public int getOrderNumber() {
}
/**
- * Shared switch point invalidated when metaclass state changes.
+ * Legacy process-wide SwitchPoint retained for binary compatibility only.
+ * <p>
+ * <strong>Behavioral change in 6.0 (GROOVY-12191):</strong> MetaClass
changes
+ * are scoped per {@link org.codehaus.groovy.reflection.ClassInfo}. This
field
+ * is <em>not</em> the MOP guard on linked call sites and is <em>not</em>
+ * rotated on per-class MetaClass changes. It is rotated only when
+ * {@link #invalidateSwitchPoints()} runs — i.e. category enter/leave and
+ * {@code VMPlugin.invalidateCallSites()} — so external observers of this
+ * field still see those bulk events. Guarding a site on this field alone
+ * will <strong>silently miss</strong> type-scoped MetaClass invalidations;
+ * migrate to {@link IndyInvalidation#guardWithMopSwitchPoints}.
+ *
+ * @see IndyInvalidation
+ * @deprecated since 6.0.0 — use {@link
IndyInvalidation#guardWithMopSwitchPoints};
+ * this field is not the call-site MOP guard and is not
rotated on
+ * per-class MetaClass changes.
*/
- protected static SwitchPoint switchPoint = new SwitchPoint();
+ @Deprecated(since = "6.0.0", forRemoval = false)
+ protected static volatile SwitchPoint switchPoint = new SwitchPoint();
static {
-
GroovySystem.getMetaClassRegistry().addMetaClassRegistryChangeEventListener(cmcu
-> invalidateSwitchPoints());
+ // MetaClass registry changes invalidate the affected class + subtypes
(GROOVY-12191).
+ // Hierarchy fan-out is owned here (not duplicated in
setStrongMetaClass beyond a
+ // local retire of the exact class for paths that never fire the
registry).
+
GroovySystem.getMetaClassRegistry().addMetaClassRegistryChangeEventListener(cmcu
-> {
+ Class<?> type = cmcu.getClassToUpdate();
+ if (type != null) {
+ IndyInvalidation.invalidateClass(type);
+ if (LOG_ENABLED) {
+ LOG.info("invalidating class SwitchPoint hierarchy for " +
type.getName());
+ }
+ } else {
+ IndyInvalidation.invalidateUnscoped();
+ if (LOG_ENABLED) {
+ LOG.info("unscoped SwitchPoint invalidation (unattributed
MetaClass change)");
+ }
+ }
+ });
}
/**
- * Callback for constant metaclass update change
+ * Category enter/leave and {@code VMPlugin.invalidateCallSites()}.
+ * Bulk-invalidates every loaded class SwitchPoint so sites re-link under
the
+ * new category state, and rotates the legacy {@link #switchPoint} field
under
+ * {@code synchronized (IndyInterface.class)} so concurrent bulk
invalidations
+ * cannot orphan a live SwitchPoint an external reader just observed.
+ * Per-class MetaClass changes use {@link
IndyInvalidation#invalidateClass(Class)}
+ * (or {@link org.codehaus.groovy.reflection.ClassInfo#incVersion()})
instead
+ * and do <em>not</em> rotate {@link #switchPoint}.
*/
protected static void invalidateSwitchPoints() {
if (LOG_ENABLED) {
- LOG.info("invalidating switch point");
+ LOG.info("invalidating class SwitchPoints for category /
invalidateCallSites");
}
-
+ IndyInvalidation.invalidateCategory();
+ // Binary-compat: rotate the legacy field so external observers of bulk
+ // invalidation still see a change. Synchronized so two concurrent
callers
+ // cannot leave a reader holding a live SwitchPoint that will never be
+ // invalidated again (pre-6.0 shape; addresses Sonar S3077 /
GROOVY-12191).
synchronized (IndyInterface.class) {
SwitchPoint old = switchPoint;
switchPoint = new SwitchPoint();
- SwitchPoint.invalidateAll(new SwitchPoint[]{old});
+ if (old != null && !old.hasBeenInvalidated()) {
+ SwitchPoint.invalidateAll(new SwitchPoint[]{old});
+ }
}
}
+ /**
+ * Installs the per-class MOP SwitchPoint guard on a linked handle.
+ *
+ * @param handle fast-path handle
+ * @param fallback re-link handle
+ * @param receiver call receiver (may be {@code null})
+ * @return guarded handle
+ */
+ static MethodHandle applyMopSwitchPoints(final MethodHandle handle, final
MethodHandle fallback, final Object receiver) {
+ return IndyInvalidation.guardWithMopSwitchPoints(handle, fallback,
receiver);
+ }
Review Comment:
what is the rationale for having the guard creation in IndyInvalidation
instead of here.
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
@@ -215,29 +216,86 @@ public int getOrderNumber() {
}
/**
- * Shared switch point invalidated when metaclass state changes.
+ * Legacy process-wide SwitchPoint retained for binary compatibility only.
+ * <p>
+ * <strong>Behavioral change in 6.0 (GROOVY-12191):</strong> MetaClass
changes
+ * are scoped per {@link org.codehaus.groovy.reflection.ClassInfo}. This
field
+ * is <em>not</em> the MOP guard on linked call sites and is <em>not</em>
+ * rotated on per-class MetaClass changes. It is rotated only when
+ * {@link #invalidateSwitchPoints()} runs — i.e. category enter/leave and
+ * {@code VMPlugin.invalidateCallSites()} — so external observers of this
+ * field still see those bulk events. Guarding a site on this field alone
+ * will <strong>silently miss</strong> type-scoped MetaClass invalidations;
+ * migrate to {@link IndyInvalidation#guardWithMopSwitchPoints}.
+ *
+ * @see IndyInvalidation
+ * @deprecated since 6.0.0 — use {@link
IndyInvalidation#guardWithMopSwitchPoints};
+ * this field is not the call-site MOP guard and is not
rotated on
+ * per-class MetaClass changes.
*/
- protected static SwitchPoint switchPoint = new SwitchPoint();
+ @Deprecated(since = "6.0.0", forRemoval = false)
+ protected static volatile SwitchPoint switchPoint = new SwitchPoint();
Review Comment:
all under vmplugin is supposed to be Internal, thus removing instead of
deprecating is imho very much allowed. Also why do you make it volatile if you
deprecate it?
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
@@ -472,9 +537,14 @@ private static Object
invokeColdReflective(ColdReflectiveMethodHandleWrapper col
throw ScriptBytecodeAdapter.unwrap(gre);
}
}
- MethodHandle mh = selectMethodHandle(cold.callSite, cold.sender,
cold.methodName, cold.callID,
- cold.safeNavigation, cold.thisCall, cold.spreadCall, 1,
arguments);
- return mh.invokeExact(arguments);
+ // Re-select without the cold tier so an always-invalid SwitchPoint
(or any
+ // permanent cold miss after class-domain failover) cannot recurse
through
+ // tryBuild → invokeColdReflective (GROOVY-12191).
+ MethodHandleWrapper full = fallback(cold.callSite, cold.sender,
cold.methodName, cold.callID,
+ cold.safeNavigation, cold.thisCall, cold.spreadCall, 1,
arguments, false);
+ cold.callSite.put(receiverCacheKey(arguments[0]),
+ full.isCanSetTarget() ? full : NULL_METHOD_HANDLE_WRAPPER);
Review Comment:
why is NULL_METHOD_HANDLE_WRAPPER the fallback for if cannot set target? It
may be because of the code path to here, but frankly I think this needs at
least a comment as of why this is the only valid version.
> Scope indy SwitchPoint invalidation
> -----------------------------------
>
> Key: GROOVY-12191
> URL: https://issues.apache.org/jira/browse/GROOVY-12191
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
> Fix For: 6.0.0-beta-1
>
>
> h3. Problem
> With invokedynamic enabled (default since Groovy 4), linked MOP call sites
> were guarded by a *single process-wide* {{SwitchPoint}}
> ({{{}IndyInterface.switchPoint{}}}).
> Any MetaClass registry change or category enter/leave invalidated that switch
> point, so *every* linked site fell back and re-linked — including sites whose
> receiver type was unrelated.
> That global invalidation is expensive when MetaClass churn is common (e.g.
> ExpandoMetaClass / mixins on startup or per-request paths, Grails-like
> patterns). Unrelated hot monomorphic sites pay re-link and JIT deopt cost
> they should not.
> h3. Goal
> Keep linked call sites optimized unless the *relevant* MetaClass state for
> that site actually changed.
> h3. Approach
> One SwitchPoint domain {*}per class{*}, stored on {{{}ClassInfo{}}}:
> * MetaClass change for type {{T}} retires {{{}T{}}}'s SwitchPoint *and*
> those of loaded subtypes / implementors (hierarchy fan-out).
> * Unrelated types keep their SwitchPoints; their call sites stay optimized.
> * Category enter/leave (and {{{}VMPlugin.invalidateCallSites(){}}})
> bulk-retire *all* loaded class SwitchPoints so sites re-link under the new
> category state. There is *no* second category SwitchPoint on the hot path.
> * Linked handles always install a *single* class-domain guard via
> {{IndyInvalidation.guardWithMopSwitchPoints(...)}} — same monomorphic guard
> shape as before, without global deopt on unrelated MetaClass churn.
> Final classes short-circuit hierarchy fan-out (no full {{ClassInfo}} scan).
> Non-final types batch retirements with {{{}SwitchPoint.invalidateAll{}}}.
> h3. Invalidation map
> ||Event||What is retired||
> |MetaClass change for type {{T}} (registry /
> {{{}ClassInfo.incVersion{}}})|{{T}} + loaded subtypes / implementors|
> |Category enter/leave, {{invalidateCallSites()}}|All loaded class
> SwitchPoints (bulk)|
> |Unattributed MetaClass registry event|All loaded class SwitchPoints (bulk)|
> |First MetaClass *install* on a class|Version bump only (no linked sites
> yet); replacement / clear retires that class's SwitchPoint|
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
