[
https://issues.apache.org/jira/browse/GROOVY-10307?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18055829#comment-18055829
]
ASF GitHub Bot commented on GROOVY-10307:
-----------------------------------------
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.
> Groovy 4 runtime performance on average 2.4x slower than Groovy 3
> -----------------------------------------------------------------
>
> Key: GROOVY-10307
> URL: https://issues.apache.org/jira/browse/GROOVY-10307
> Project: Groovy
> Issue Type: Bug
> Components: bytecode, performance
> Affects Versions: 4.0.0-beta-1, 3.0.9
> Environment: OpenJDK Runtime Environment AdoptOpenJDK-11.0.11+9
> (build 11.0.11+9)
> OpenJDK 64-Bit Server VM AdoptOpenJDK-11.0.11+9 (build 11.0.11+9, mixed mode)
> WIN10 (tests) / REL 8 (web application)
> IntelliJ 2021.2
> Reporter: mgroovy
> Priority: Major
> Attachments: groovy_3_0_9_gc.png, groovy_3_0_9_loop2.png,
> groovy_3_0_9_loop4.png, groovy_3_0_9_mem.png, groovy_4_0_0_b1_loop2.png,
> groovy_4_0_0_b1_loop4.png, groovy_4_0_0_b1_loop4_gc.png,
> groovy_4_0_0_b1_loop4_mem.png,
> groovysql_performance_groovy4_2_xx_yy_zzzz.groovy, loops.groovy,
> profile3.txt, profile4-loops.txt, profile4.txt, profile4d.txt
>
>
> Groovy 4.0.0-beta-1 runtime performance in our framework is on average 2 to 3
> times slower compared to using Groovy 3.0.9 (regular i.e. non-INDY)
> * Our complete framework and application code is completely written in
> Groovy, spread over multiple IntelliJ modules
> ** mixed @CompileDynamic/@TypeChecked and @CompileStatic
> ** No Java classes left in project, i.e. no cross compilation occurs
> * We build using IntelliJ 2021.2 Groovy build process, then run / deploy the
> compiled class files
> ** We do _not_ use a Groovy based DSL, nor do we execute Groovy scripts
> during execution
> * Performance degradation when using Groovy 4.0.0-beta-1 instead of Groovy
> 3.0.9 (non-INDY):
> ** The performance of the largest of our web applications has dropped 3x
> (startup) / 2x (table refresh) respectively
> *** Stack: Tomcat/Vaadin/Ebean plus framework generated SQL
> ** Our test suite runs about 2.4 times as long as before (120 min when using
> G4, compared to about 50 min with G3)
> *** JUnit 5
> *** test suite also contains no scripts / dynamic code execution
> *** Individual test performance varies: A small number of tests runs faster,
> but the majority is slower, with some extreme cases taking nearly 10x as long
> to finish
> * Using Groovy 3.0.9 INDY displays nearly identical performance degradation,
> so it seems that the use of invoke dynamic is somehow at fault
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
