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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
