-
Notifications
You must be signed in to change notification settings - Fork 1.9k
GROOVY-10307: Improve invokedynamic performance with optimized caching #2374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
510762d
74f0798
43d99f9
ca36b8c
8c05bd5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -32,7 +32,10 @@ | |||||||||||
| import java.lang.invoke.MethodType; | ||||||||||||
| import java.lang.invoke.MutableCallSite; | ||||||||||||
| import java.lang.invoke.SwitchPoint; | ||||||||||||
| import java.lang.ref.WeakReference; | ||||||||||||
| import java.util.Map; | ||||||||||||
| import java.util.Set; | ||||||||||||
| import java.util.concurrent.ConcurrentHashMap; | ||||||||||||
| import java.util.function.BiFunction; | ||||||||||||
| import java.util.function.Function; | ||||||||||||
| import java.util.logging.Level; | ||||||||||||
|
|
@@ -168,24 +171,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(); | ||||||||||||
|
jamesfredley marked this conversation as resolved.
Outdated
|
||||||||||||
|
|
||||||||||||
| 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)); | ||||||||||||
|
jamesfredley marked this conversation as resolved.
|
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * 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 -> { | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The size of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Current implementation will clear all cache no matter whether the changed metaclasses are releated.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tested two versions of the parallel stream change with the two test application and saw a reduction in performance.
Approach 1: Parallel Collect + Atomic Swap - 18% SLOWER Approach 2: Parallel ForEach + Sequential RemoveIf - 12% SLOWER
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Testing with the Groovy 5 caller-specific Lookup pattern is generally positive on the micro benchmarks but 11% slower in the grails test application: More details: jamesfredley#1 |
||||||||||||
| 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(); | ||||||||||||
|
jamesfredley marked this conversation as resolved.
|
||||||||||||
| return false; | ||||||||||||
| }); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
|
|
@@ -230,6 +264,9 @@ private static CallSite realBootstrap(Lookup caller, String name, int callID, Me | |||||||||||
| mc.setTarget(mh); | ||||||||||||
| mc.setDefaultTarget(mh); | ||||||||||||
| mc.setFallbackTarget(makeFallBack(mc, sender, name, callID, type, safe, thisCall, spreadCall)); | ||||||||||||
|
|
||||||||||||
| // Register for cache invalidation on metaclass changes | ||||||||||||
| registerCallSite(mc); | ||||||||||||
|
|
||||||||||||
| return mc; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ | |
| import groovy.lang.MissingMethodException; | ||
| import groovy.transform.Internal; | ||
| import org.apache.groovy.runtime.ObjectUtil; | ||
| import org.apache.groovy.util.SystemUtil; | ||
| import org.codehaus.groovy.GroovyBugError; | ||
| import org.codehaus.groovy.reflection.CachedField; | ||
| import org.codehaus.groovy.reflection.CachedMethod; | ||
|
|
@@ -940,9 +941,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 | ||
|
jamesfredley marked this conversation as resolved.
Outdated
|
||
| if (SystemUtil.getBooleanSafe("groovy.indy.switchpoint.guard")) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The one question I have about this is whether this check ought to be inverted. That is, should the default behaviour be the old way, which we know is less performant but more battle-tested, or this way? Could Grails simply turn this flag on by default, thus solving their problem, without impacting other users? Also, as a nitpicky thing, I reckon this string ought to be a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this PRs approach is viable, but does not prove to be generally valuable for all/most code run with Indy on, than I think the inverse would be fine for Grails and we could set that flag in default generated applications.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is part of why I'd like to run this branch against the current benchmarks on CI. That would likely provide some confidence as to whether this was "safe" to merge without impacting anyone's expectations about how the language behaves.
jamesfredley marked this conversation as resolved.
Outdated
|
||
| handle = switchPoint.guardWithTest(handle, fallback); | ||
| if (LOG_ENABLED) LOG.info("added switch point guard"); | ||
| } | ||
|
|
||
| java.util.function.Predicate<Class<?>> nonFinalOrNullUnsafe = (t) -> { | ||
| return !Modifier.isFinal(t.getModifiers()) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.