[
https://issues.apache.org/jira/browse/GROOVY-10307?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18056538#comment-18056538
]
ASF GitHub Bot commented on GROOVY-10307:
-----------------------------------------
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);
}
```
> 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)
