Copilot commented on code in PR #2374:
URL: https://github.com/apache/groovy/pull/2374#discussion_r2751925765
##########
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:
The WeakReference-based call site registry could accumulate
garbage-collected entries over time if metaclass changes are infrequent. While
`removeIf` cleans up stale references during invalidation, if metaclass changes
rarely occur in an application, the set could grow with dead references.
Consider adding periodic cleanup or a size-based trigger to remove stale
WeakReferences. For example, you could check the set size periodically (e.g.,
every N registrations) and clean up if it exceeds a threshold. Alternatively,
document this as acceptable behavior if the memory overhead is negligible.
##########
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:
The documentation states that the SwitchPoint guard is disabled "by default"
and can be re-enabled, but the implications of doing so are not clearly
explained. Since the PR description emphasizes that the SwitchPoint causes ALL
call sites to fail when ANY metaclass changes, enabling it would reintroduce
the performance problem this PR aims to solve.
Consider adding a warning in the comment that enabling this property will
significantly degrade performance in frameworks with frequent metaclass changes
(like Grails), and that it should only be enabled for specific debugging or
compatibility scenarios. This will help prevent users from accidentally
negating the performance improvements.
```suggestion
* Whether to use a global SwitchPoint guard for metaclass changes.
* <p>
* <strong>Default is {@code false}</strong> to avoid the performance
cost of globally
* invalidating all indy call sites whenever <em>any</em> metaclass
changes.
* Enabling this will significantly degrade performance in frameworks or
applications
* with frequent metaclass changes (for example, Grails), because every
such change
* forces all guarded call sites to be re-linked.
* <p>
* Set {@code groovy.indy.switchpoint.guard=true} <strong>only</strong>
for specific
* debugging or backward-compatibility scenarios where strict metaclass
change
* detection is required, and not for general production use.
```
##########
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:
The call site cache invalidation logic has a potential race condition
between checking and setting the target. Between lines 223 and 224, another
thread could modify the call site's target, causing the comparison to become
stale. While MutableCallSite.setTarget() is thread-safe according to the JDK
documentation (it provides volatile semantics), the check-then-set pattern is
not atomic.
Additionally, calling `cs.setTarget()` and `cs.clearCache()` as separate
operations is not atomic. A thread could invoke the call site between these two
operations, potentially using a reset target with stale cached data.
Consider either: (1) accepting this race as benign (if a call site is
actively being invoked during invalidation, it will get the old behavior for
one more call, which might be acceptable), or (2) adding synchronization to
make the entire invalidation atomic per call site. Document the chosen approach
and its implications.
```suggestion
// Invalidate all call site caches and reset targets to default
(cache lookup)
// This ensures metaclass changes are visible without using
expensive switchpoint guards.
// Synchronize per call site so that checking the target, resetting
it, and clearing the cache
// happen atomically with respect to that call site, avoiding races
during invalidation.
ALL_CALL_SITES.removeIf(ref -> {
CacheableCallSite cs = ref.get();
if (cs == null) {
return true; // Remove garbage collected references
}
synchronized (cs) {
// 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();
}
```
##########
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:
The `clearCache()` method has a potential race condition with concurrent
calls to `getAndPut()`. The `latestHitMethodHandleWrapperSoftReference` field
is marked as volatile, but setting it to null here is not synchronized. A
thread could be in the middle of `getAndPut()` reading this field (lines 75-84
in the original file) while another thread calls `clearCache()`. This could
lead to inconsistent state.
Consider wrapping the assignment `latestHitMethodHandleWrapperSoftReference
= null` inside the synchronized block to ensure atomicity with the lruCache
operations, or use a more sophisticated approach like AtomicReference for the
soft reference field.
```suggestion
// Clear the latest hit reference and the LRU cache atomically
synchronized (lruCache) {
latestHitMethodHandleWrapperSoftReference = null;
```
##########
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:
The inline comment uses inconsistent style with the rest of the method. The
comment spans lines 951-958, but the actual conditional logic it describes
starts at line 959. The multi-line comment breaks up the flow of the
guard-setting logic.
Consider moving this detailed explanation to the field declaration
(INDY_SWITCHPOINT_GUARD) or method-level documentation, and keeping only a
brief inline comment here such as "// Apply global switchpoint guard if enabled
(disabled by default for performance)". This would be more consistent with the
other guard comments in this method (e.g., lines 926, 931, 947).
```suggestion
// Apply global switchpoint guard if enabled (disabled by
default for performance)
```
--
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]
