GROOVY-12165: extend Closure.call fast path to multi-argument overrides#2712
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends Closure.call’s cached fast path from zero/one-argument overrides to multi-argument overrides by caching call targets per-arity (0..4) and using per-parameter instanceof guards to preserve existing coercion/dispatch semantics via the metaclass fallback.
Changes:
- Generalize the call-override cache to an arity-indexed table with per-argument guards, covering common GDK-driven multi-arg call shapes (e.g., map iteration,
inject,eachWithIndex). - Simplify reflective invocation to pass the existing argument array directly, avoiding re-wrapping for 1-arg fast-path calls.
- Add regression tests for multi-argument fast-path dispatch, guard fall-through (coercion/null), overload ambiguity, static
callexclusions, and default-parameter arity population.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/main/java/groovy/lang/Closure.java | Reworks Closure.call fast-path caching to be arity-indexed with guard checks, preserving metaclass fallback semantics. |
| src/test/groovy/bugs/Groovy12165.groovy | Adds JUnit tests to validate multi-arg call fast-path behavior and guard-based fall-through across common GDK call patterns. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2712 +/- ##
==================================================
+ Coverage 69.1120% 69.1164% +0.0044%
+ Complexity 34251 34250 -1
==================================================
Files 1537 1537
Lines 129403 129457 +54
Branches 23526 23547 +21
==================================================
+ Hits 89433 89476 +43
- Misses 31944 31952 +8
- Partials 8026 8029 +3
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
The cached fast path covered only zero- and one-argument call overrides
(GROOVY-11911, GROOVY-12164), so every multi-argument closure call from
Java -- all Map iteration, eachWithIndex, inject -- took full metaclass
dispatch. Generalise the cache to an arity-indexed table (0..4) with
per-argument instance-of guards: at each arity an all-Object override
wins outright (the 11911 rule), otherwise the single unambiguous typed
override dispatches when every argument is already an instance of its
declared type (the 12164 rule), and anything else (coercion, null,
same-arity overloads, static declarations, array params) falls through
to the metaclass exactly as before. Map#each is ~3-3.6x faster, inject
~2x, eachWithIndex ~1.65x; one-arg calls also gain ~13% by spreading
the argument array directly instead of re-wrapping it.
GROOVY-12165: key the cache on doCall and guard it on the stock metaclass
Per the closure-dispatch design discussion: call selects a doCall, so the
arity-indexed cache is re-keyed from call overloads to the subclass's
doCall declarations (hierarchy walk, most-derived override wins, vararg/
static/bridge shapes and same-arity overloads decline to the metaclass).
Declared call()/call(Object) overrides keep per-arity precedence
(GROOVY-11911) -- DGM's closure.call(item) reaches such an override via
plain virtual dispatch, so the varargs entry must agree -- while typed or
multi-arg call overloads are no longer honoured, restoring 3.x-5.x
behaviour (the ecosystem census found zero reliance: every real subclass
either overrides call(Object...) or declares doCall). MethodClosure and
CurriedClosure are excluded: MetaClassImpl has dedicated dispatch for
them, so their doCall declarations are not what the MOP would select.
The cache is now also semantically invisible: a mopPerturbed flag
(checked at construction, latched by setMetaClass -- the only mutation
path for the instance's metaclass field) plus the category quick-exit
gate the fast path, so a per-instance metaclass or active category
routes through invokeMethod as it did before the cache existed. The
re-entry latch is scoped to the 11911 call-form targets only: doCall
targets skip the ThreadLocal entirely, so nested closure calls inside a
dispatched body keep their own fast path (sum-style shapes gain ~50%).
Also keeps the GString-key coercion test honest (Copilot review): the
key stays a GString so the guard genuinely fails and the metaclass
coercion path is exercised.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The cached fast path covered only zero- and one-argument call overrides (GROOVY-11911, GROOVY-12164), so every multi-argument closure call from Java -- all Map iteration, eachWithIndex, inject -- took full metaclass dispatch. Generalise the cache to an arity-indexed table (0..4) with per-argument instance-of guards: at each arity an all-Object override wins outright (the 11911 rule), otherwise the single unambiguous typed override dispatches when every argument is already an instance of its declared type (the 12164 rule), and anything else (coercion, null, same-arity overloads, static declarations, array params) falls through to the metaclass exactly as before. Map#each is ~3-3.6x faster, inject ~2x, eachWithIndex ~1.65x; one-arg calls also gain ~13% by spreading the argument array directly instead of re-wrapping it.