jamesfredley commented on code in PR #2374:
URL: https://github.com/apache/groovy/pull/2374#discussion_r2752090064
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
@@ -168,24 +178,55 @@ public static CallType fromCallSiteName(String
callSiteName) {
}
protected static SwitchPoint switchPoint = new SwitchPoint();
+
+ /**
+ * Weak set of all CacheableCallSites. Used to invalidate caches when
metaclass changes.
+ * Uses WeakReferences so call sites can be garbage collected when no
longer referenced.
+ */
+ private static final Set<WeakReference<CacheableCallSite>> ALL_CALL_SITES
= ConcurrentHashMap.newKeySet(INDY_CALLSITE_INITIAL_CAPACITY);
static {
GroovySystem.getMetaClassRegistry().addMetaClassRegistryChangeEventListener(cmcu
-> invalidateSwitchPoints());
}
+
+ /**
+ * Register a call site for cache invalidation when metaclass changes.
+ */
+ static void registerCallSite(CacheableCallSite callSite) {
+ ALL_CALL_SITES.add(new WeakReference<>(callSite));
+ }
/**
- * Callback for constant metaclass update change
+ * Callback for constant metaclass update change.
+ * Invalidates all call site caches to ensure metaclass changes are
visible.
*/
protected static void invalidateSwitchPoints() {
if (LOG_ENABLED) {
- LOG.info("invalidating switch point");
+ LOG.info("invalidating switch point and call site caches");
}
synchronized (IndyInterface.class) {
SwitchPoint old = switchPoint;
switchPoint = new SwitchPoint();
SwitchPoint.invalidateAll(new SwitchPoint[]{old});
}
+
+ // Invalidate all call site caches and reset targets to default (cache
lookup)
+ // This ensures metaclass changes are visible without using expensive
switchpoint guards
+ ALL_CALL_SITES.removeIf(ref -> {
+ CacheableCallSite cs = ref.get();
+ if (cs == null) {
+ return true; // Remove garbage collected references
+ }
+ // Reset target to default (fromCache) so next call goes through
cache lookup
+ MethodHandle defaultTarget = cs.getDefaultTarget();
+ if (defaultTarget != null && cs.getTarget() != defaultTarget) {
+ cs.setTarget(defaultTarget);
+ }
+ // Clear the cache so stale method handles are discarded
+ cs.clearCache();
Review Comment:
Fixed in commit 6c6593e25d. Added `synchronized(cs)` block around the target
check, target reset, and cache clear operations to ensure atomicity. This
prevents race conditions where another thread could modify the call site
between checking and setting the target.
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
@@ -168,24 +178,55 @@ public static CallType fromCallSiteName(String
callSiteName) {
}
protected static SwitchPoint switchPoint = new SwitchPoint();
+
+ /**
+ * Weak set of all CacheableCallSites. Used to invalidate caches when
metaclass changes.
+ * Uses WeakReferences so call sites can be garbage collected when no
longer referenced.
+ */
+ private static final Set<WeakReference<CacheableCallSite>> ALL_CALL_SITES
= ConcurrentHashMap.newKeySet(INDY_CALLSITE_INITIAL_CAPACITY);
static {
GroovySystem.getMetaClassRegistry().addMetaClassRegistryChangeEventListener(cmcu
-> invalidateSwitchPoints());
}
+
+ /**
+ * Register a call site for cache invalidation when metaclass changes.
+ */
+ static void registerCallSite(CacheableCallSite callSite) {
+ ALL_CALL_SITES.add(new WeakReference<>(callSite));
Review Comment:
Added comprehensive Javadoc in commit 6c6593e25d documenting the cleanup
behavior. Stale WeakReferences are cleaned up during each call to
`invalidateSwitchPoints()`. In applications with infrequent metaclass changes,
some dead references may accumulate, but this is acceptable because: (1) memory
overhead per dead reference is minimal (~16 bytes), and (2) frameworks like
Grails that benefit most from this optimization have frequent metaclass changes
that trigger regular cleanup.
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
@@ -124,6 +125,13 @@ public abstract class Selector {
*/
private static final CallType[] CALL_TYPE_VALUES = CallType.values();
+ /**
+ * Whether to use global SwitchPoint guard for metaclass changes.
+ * Default is false for better performance in frameworks with frequent
metaclass changes.
+ * Set groovy.indy.switchpoint.guard=true to enable strict metaclass
change detection.
Review Comment:
Added detailed Javadoc in commit 6c6593e25d explaining the performance
implications. The documentation now explicitly warns that enabling the global
SwitchPoint guard will significantly degrade performance in frameworks with
frequent metaclass changes (like Grails), and recommends using it only for
specific debugging or backward-compatibility scenarios.
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
@@ -940,9 +948,18 @@ public void setGuards(Object receiver) {
}
}
- // handle constant metaclass and category changes
- handle = switchPoint.guardWithTest(handle, fallback);
- if (LOG_ENABLED) LOG.info("added switch point guard");
+ // Skip the global switchpoint guard by default.
+ // The switchpoint causes ALL call sites to fail when ANY
metaclass changes.
+ // In Grails and similar frameworks with frequent metaclass
changes, this causes
+ // massive guard failures and performance degradation.
+ // The other guards (metaclass identity, class receiver, category)
should be
+ // sufficient, combined with cache invalidation on metaclass
changes.
+ //
+ // If you need strict metaclass change detection, set
groovy.indy.switchpoint.guard=true
Review Comment:
Simplified the inline comment in commit 6c6593e25d to a single line: "Apply
global switchpoint guard if enabled (disabled by default for performance)". The
detailed explanation of why the guard is disabled by default was moved to the
`INDY_SWITCHPOINT_GUARD` field's Javadoc, which is the more appropriate
location for this documentation.
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/CacheableCallSite.java:
##########
@@ -124,4 +124,21 @@ public MethodHandle getFallbackTarget() {
public void setFallbackTarget(MethodHandle fallbackTarget) {
this.fallbackTarget = fallbackTarget;
}
+
+ /**
+ * Clear the cache entirely. Called when metaclass changes to ensure
+ * stale method handles are discarded.
+ */
+ public void clearCache() {
+ // Clear the latest hit reference
+ latestHitMethodHandleWrapperSoftReference = null;
+
+ // Clear the LRU cache
+ synchronized (lruCache) {
Review Comment:
Fixed in commit 6c6593e25d. Moved the
`latestHitMethodHandleWrapperSoftReference = null` assignment inside the
`synchronized(lruCache)` block to ensure atomicity with concurrent
`getAndPut()` operations. The `fallbackCount.set(0)` remains outside the sync
block since AtomicInteger operations are inherently thread-safe.
--
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]
