jamesfredley commented on code in PR #2374:
URL: https://github.com/apache/groovy/pull/2374#discussion_r2752174028
##########
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:
**Update:** After further analysis with multiple reviewers, we decided NOT
to add the `synchronized(cs)` block. Here's why:
1. **No other code synchronizes on `cs`** - The threads calling
`setTarget()` in `fromCache()` and `selectMethod()` don't lock `cs`, so our
lock doesn't make the operation atomic relative to them.
2. **The operation is idempotent** - Setting target to `defaultTarget`
multiple times is harmless.
3. **Guards provide the real safety** - Even if a concurrent thread briefly
reinstalls an optimized target after we reset it, the method handle guards
(SAME_MC, SAME_CLASS) will fail on the next call if the metaclass changed,
forcing a fallback to `selectMethod()`.
4. **Overhead without benefit** - The synchronized block adds monitor
enter/exit overhead for no real safety gain.
Updated in commit 8c05bd5f46 with documentation explaining the best-effort
invalidation strategy.
##########
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:
**Update:** After analysis, we're keeping the volatile field access
**outside** the synchronized block (matching the original code pattern).
The field `latestHitMethodHandleWrapperSoftReference` is a **best-effort
optimization heuristic** for tracking hit counts, not a correctness mechanism.
The race between `clearCache()` and `getAndPut()` is benign - worst case is
slightly skewed hit count statistics.
Since `getAndPut()` already accesses this volatile field outside the lock
(lines 75-84), putting the null assignment inside the lock in `clearCache()`
doesn't provide atomicity anyway.
Updated in commit 8c05bd5f46 with documentation explaining this design
choice.
--
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]
