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