Copilot commented on code in PR #2377:
URL: https://github.com/apache/groovy/pull/2377#discussion_r2766032370
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/CacheableCallSite.java:
##########
@@ -123,6 +123,18 @@ public void resetFallbackCount() {
fallbackRound.incrementAndGet();
}
+ /**
+ * Clear the LRU cache and reset fallback count.
+ * Called when metaclass changes to ensure stale method handles are
discarded.
+ */
+ public void clearCache() {
+ synchronized (lruCache) {
+ lruCache.clear();
+ }
+ latestHitMethodHandleWrapperSoftReference = null;
Review Comment:
The `clearCache()` method has a potential race condition with the
`latestHitMethodHandleWrapperSoftReference` field. The field is declared as
`volatile` (line 48) and is accessed outside the synchronized block (line 134).
This creates a race condition where:
1. Thread A could be in `getAndPut()` reading
`latestHitMethodHandleWrapperSoftReference` at line 84
2. Thread B calls `clearCache()` and sets
`latestHitMethodHandleWrapperSoftReference = null` at line 134
3. Thread A proceeds with stale data and might write back the reference at
line 92
The assignment to `latestHitMethodHandleWrapperSoftReference` should be done
inside the synchronized block to ensure atomicity with the cache clearing
operation.
```suggestion
latestHitMethodHandleWrapperSoftReference = null;
}
```
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
@@ -181,24 +189,54 @@ public int getOrderNumber() {
}
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 cleanup relies entirely on `invalidateSwitchPoints()`
being called to remove garbage-collected references. If metaclass changes are
infrequent or never occur in a long-running application, the `ALL_CALL_SITES`
set could accumulate a large number of cleared WeakReferences (where `ref.get()
== null`), consuming memory unnecessarily.
Consider adding a periodic cleanup mechanism or implementing cleanup on
registration if the set size exceeds a threshold. For example, you could check
the set size in `registerCallSite()` and trigger cleanup if it exceeds a
configurable threshold, similar to how other Groovy components handle weak
reference cleanup.
```suggestion
/**
* Threshold for triggering cleanup of cleared WeakReferences from
ALL_CALL_SITES.
* Configurable via system property
"groovy.indy.callsites.cleanup.threshold".
*/
private static final long ALL_CALL_SITES_CLEANUP_THRESHOLD =
SystemUtil.getLongSafe("groovy.indy.callsites.cleanup.threshold", 10_000L);
static {
GroovySystem.getMetaClassRegistry().addMetaClassRegistryChangeEventListener(cmcu
-> invalidateSwitchPoints());
}
/**
* Remove entries from ALL_CALL_SITES whose referents have been
garbage-collected.
*/
private static void cleanupClearedCallSites() {
ALL_CALL_SITES.removeIf(ref -> ref.get() == null);
}
/**
* Register a call site for cache invalidation when metaclass changes.
*/
static void registerCallSite(CacheableCallSite callSite) {
ALL_CALL_SITES.add(new WeakReference<>(callSite));
// Periodically clean up cleared WeakReferences to avoid unbounded
growth
if (ALL_CALL_SITES.size() > ALL_CALL_SITES_CLEANUP_THRESHOLD) {
cleanupClearedCallSites();
}
```
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java:
##########
@@ -181,24 +189,54 @@ public int getOrderNumber() {
}
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).
+ 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);
Review Comment:
The `invalidateSwitchPoints()` method has a potential race condition when
resetting call site targets. The method reads the defaultTarget and
currentTarget, then conditionally sets the target, all without synchronization
on the CacheableCallSite object. This creates a race where:
1. Thread A could be calling `setTarget()` on the call site (e.g., during
optimization)
2. Thread B (invalidation) reads `cs.getTarget()` and
`cs.getDefaultTarget()`
3. Thread A completes its `setTarget()` call
4. Thread B then calls `cs.setTarget(defaultTarget)`, potentially
overwriting Thread A's update
Since `MutableCallSite.setTarget()` is synchronized on the call site itself,
consider synchronizing on the call site object when checking and resetting the
target to ensure atomicity.
```suggestion
synchronized (cs) {
MethodHandle defaultTarget = cs.getDefaultTarget();
MethodHandle currentTarget = cs.getTarget();
if (defaultTarget != null && currentTarget != defaultTarget)
{
cs.setTarget(defaultTarget);
}
```
--
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]
