GROOVY-10307: Address PR review feedback for thread safety and documentation

- Fix race condition in invalidateSwitchPoints(): synchronize target reset
  and cache clear operations per call site to ensure atomicity
- Fix race condition in clearCache(): move latestHitMethodHandleWrapper
  null assignment inside synchronized block for consistency with getAndPut
- Add comprehensive Javadoc for INDY_SWITCHPOINT_GUARD explaining
  performance implications of enabling the global switchpoint guard
- Document WeakReference cleanup behavior in ALL_CALL_SITES and
  registerCallSite() to clarify garbage collection semantics
- Simplify inline comment in setGuards() with detailed Javadoc on field

Author: jamesfredley

src/main/java/org/codehaus/groovy/vmplugin/v8/CacheableCallSite.java

@@ -128,17 +128,18 @@ public void setFallbackTarget(MethodHandle fallbackTarget) {
     /**
      * Clear the cache entirely. Called when metaclass changes to ensure
      * stale method handles are discarded.
+     * <p>
+     * This method synchronizes on the lruCache to ensure atomicity with
+     * concurrent {@link #getAndPut} operations.
      */
     public void clearCache() {
-        // Clear the latest hit reference
-        latestHitMethodHandleWrapperSoftReference = null;
-        
-        // Clear the LRU cache
+        // Clear the latest hit reference and the LRU cache atomically
         synchronized (lruCache) {
+            latestHitMethodHandleWrapperSoftReference = null;
             lruCache.clear();
         }
 
-        // Reset fallback count
+        // Reset fallback count (atomic operation, doesn't need to be in sync block)
         fallbackCount.set(0);
     }
 }

src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java

@@ -182,6 +182,13 @@ public static CallType fromCallSiteName(String callSiteName) {
     /**
      * 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.
+     * <p>
+     * Note: Stale (garbage-collected) WeakReferences are cleaned up during each call to
+     * {@link #invalidateSwitchPoints()}. In applications with infrequent metaclass changes,
+     * the set may accumulate some dead references between invalidations. This is acceptable
+     * because: (1) the 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.
      */
     private static final Set<WeakReference<CacheableCallSite>> ALL_CALL_SITES = ConcurrentHashMap.newKeySet(INDY_CALLSITE_INITIAL_CAPACITY);
 
@@ -191,6 +198,10 @@ public static CallType fromCallSiteName(String callSiteName) {
 
     /**
      * Register a call site for cache invalidation when metaclass changes.
+     * <p>
+     * Registered call sites are held via WeakReferences and will be automatically
+     * removed when garbage collected. Cleanup of stale references occurs during
+     * {@link #invalidateSwitchPoints()}.
      */
     static void registerCallSite(CacheableCallSite callSite) {
         ALL_CALL_SITES.add(new WeakReference<>(callSite));
@@ -211,20 +222,24 @@ protected static void invalidateSwitchPoints() {
             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
+        // 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
             }
-            // 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);
+            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();
             }
-            // Clear the cache so stale method handles are discarded
-            cs.clearCache();
             return false;
         });
     }

src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java

@@ -126,9 +126,17 @@ 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.
+     * 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.
      */
     private static final boolean INDY_SWITCHPOINT_GUARD = SystemUtil.getBooleanSafe("groovy.indy.switchpoint.guard");
 
@@ -948,14 +956,7 @@ public void setGuards(Object receiver) {
                 }
             }
 
-            // 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
+            // Apply global switchpoint guard if enabled (disabled by default for performance)
             if (INDY_SWITCHPOINT_GUARD) {
                 handle = switchPoint.guardWithTest(handle, fallback);
                 if (LOG_ENABLED) LOG.info("added switch point guard");