[
https://issues.apache.org/jira/browse/GROOVY-12285?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18107029#comment-18107029
]
ASF GitHub Bot commented on GROOVY-12285:
-----------------------------------------
daniellansun commented on PR #2823:
URL: https://github.com/apache/groovy/pull/2823#issuecomment-5383870142
# Response to Review on PR #2823 (GROOVY-12285)
Thank you very much for the thorough, insightful, and rigorous review. We
deeply appreciate the detailed verification of correctness (invariants,
aliasing safety, immutability, and the GROOVY-12115 cache invalidation fix), as
well as the constructive guidance to improve robustness and precision.
All feedback has been carefully analyzed and addressed in this update. Below
is the point-by-point response and a summary of the refinements made.
---
## 1. Addressing the Two Requested Changes
### (a) Eliminating the Brittle Downcast in `AbstractExtensionMethodCache`
> **Review Comment:**
> *In `AbstractExtensionMethodCache`:*
> ```java
> return methods == null ? Collections.emptyList() : ((MethodsByName)
methods).named(name);
> ```
> *This assumes every map value came through the private
`makeMethodsUnmodifiable`. True today. But `get(ClassLoader)` is **public and
non-final** on a **public abstract** class, and `MacroMethodsCache`
(groovy-macro) already subclasses it. Anyone overriding `get` — or any future
alternate population path — turns this into a `ClassCastException` deep inside
the type checker. Cheapest fix: make `get(ClassLoader)` `final`. Slightly
better: hold `Map<String, MethodsByName>` internally and widen for the public
view.*
**Resolution & Enhancements Made:**
1. **Made `get(ClassLoader)` `final`:** `public final Map<String,
List<MethodNode>> get(ClassLoader loader)` now guarantees that cache retrieval
and population lifecycle cannot be bypassed or overridden inconsistently by
subclasses.
2. **Unified Internal Structure (`MethodsByName`):** Rather than introducing
ad-hoc flags or diverging collection types across subclasses, all lists in
`AbstractExtensionMethodCache` are uniformly and immutably wrapped as
`MethodsByName`.
3. **Defensive Non-Casting Fallback:** In
`AbstractExtensionMethodCache.get(ClassLoader loader, String key, String
name)`, we added type checking and a graceful linear fallback to guarantee
complete safety against any unexpected list implementation:
```java
List<MethodNode> get(final ClassLoader loader, final String key, final
String name) {
List<MethodNode> methods = get(loader).get(key);
if (methods == null || methods.isEmpty()) {
return Collections.emptyList();
}
if (methods instanceof MethodsByName) {
return ((MethodsByName) methods).named(name);
}
// Fallback for custom/unindexed list structures
List<MethodNode> matches = new ArrayList<>(2);
for (MethodNode method : methods) {
if (method.getName().equals(name)) {
matches.add(method);
}
}
return matches.isEmpty() ? Collections.emptyList() :
Collections.unmodifiableList(matches);
}
```
---
### (b) Safety Warnings and In-Place Mutation Assertions for
`parametersForDistance`
> **Review Comment:**
> *`MethodNode.getParameters()` returns the field, not a copy. It's safe
now, but a future edit anywhere in the distance-measurement chain that writes
`parameters[i] = ...` would silently corrupt the AST for the rest of the
compile — no exception, wrong overload resolution. The javadoc mentions it; I'd
want a blunter warning at the point where the original array is returned. The
new tests assert array *identity* is preserved but not that elements are never
mutated in place. Low probability, high blast radius.*
**Resolution & Enhancements Made:**
1. **Prominent Safety Contract & Mutation Warning:** Updated Javadoc and
inline comments on `parametersForDistance(MethodNode)` and
`measureParametersAndArgumentsDistance(Parameter[], ClassNode[])` in
`StaticTypeCheckingSupport.java`:
```java
/**
* Distance measurement treats generic parameters as their erasure so a
* {@code List<T>} parameter does not reject a {@code List} argument.
* <p>
* <b>PERFORMANCE & SAFETY CONTRACT:</b> To avoid redundant array
allocations during
* overload resolution, this method reuses and returns {@link
MethodNode#getParameters()}
* directly whenever no generic erasure is needed. The returned array is
a cloned copy
* <i>only</i> when one or more parameters require generic erasure.
* <p>
* <b>CRITICAL MUTATION WARNING:</b> Callers of this method and all
downstream methods in
* the distance measurement chain (e.g. {@link
#measureParametersAndArgumentsDistance(Parameter[], ClassNode[])})
* <b>MUST NEVER</b> mutate the returned {@code Parameter[]} array or its
elements in place.
* In-place mutation would silently corrupt the {@link MethodNode}'s
parameter definitions
* for all subsequent compilation phases across the compiler.
*/
```
2. **In-Place Mutation Tests Added:** Extended
`StaticTypeCheckingSupportTest`:
- `testChooseBestMethodDoesNotMutateNonGenericParameters` now explicitly
asserts that each individual `Parameter` element in the array
(`exact.parameters[0].is(exactParamBefore)`) retains its exact instance
identity, origin type, and name.
- Added `testParameterElementsAreNeverMutatedDuringResolution` executing
complex overload resolutions (exact match, widening match, generic match,
varargs match) across candidate `MethodNode`s and asserting that every
`Parameter` element in `MethodNode.getParameters()` remains strictly identical
(`is(...)`) before and after resolution.
---
## 2. Addressing Smaller Notes
### 1. Macro Cache Index Overhead
> **Review Comment:**
> *`MacroMethodsCache.getMethodMapper()` returns `m -> m.getName()`, so its
cache keys are *method names*. `MethodsByName` then re-indexes each bucket by
the same name — a degenerate one-entry `HashMap` plus a `singletonList` per
key, for a `named()` lookup that groovy-macro never calls. Small in absolute
terms, but pure waste; a `protected boolean indexByName()` hook or lazy index
construction would avoid it.*
**Resolution:**
- Rather than adding an ad-hoc protected method/flag that would complicate
the SPI/API surface, we introduced an internal fast-path directly inside
`MethodsByName`:
```java
if (count == 0) {
this.byName = Collections.emptyMap();
} else if (count == 1) {
MethodNode m = this.methods[0];
this.byName = Collections.singletonMap(m.getName(),
Collections.singletonList(m));
} else if (allSameName(this.methods)) {
// Zero-allocation fast-path: when all methods share the same name
(e.g. MacroMethodsCache),
// byName points directly to `this` (which is already an unmodifiable
List<MethodNode>).
this.byName = Collections.singletonMap(this.methods[0].getName(),
this);
} else {
Map<String, List<MethodNode>> index = new HashMap<>(Math.max(4, (int)
(count / 0.75f) + 1));
for (MethodNode method : this.methods) {
index.computeIfAbsent(method.getName(), k -> new
ArrayList<>(2)).add(method);
}
index.replaceAll((k, v) -> v.size() == 1
? Collections.singletonList(v.get(0))
: Collections.unmodifiableList(v));
this.byName = Collections.unmodifiableMap(index);
}
```
- **Zero Overhead for Single-Name Buckets:** When all methods in a list
share the same name (which is always true for `MacroMethodsCache`), `byName`
simply creates a lightweight `singletonMap` pointing directly to `this` (the
unmodifiable list itself). No `HashMap`, no sub-lists, and no array copies are
created.
- All 66 tests in `:groovy-macro:test` pass cleanly.
---
### 2. Memory Retained Size Quantification
> **Review Comment:**
> *Memory isn't quantified. The PR quantifies everything else to the byte,
but not the retained size of the new indexes. Per receiver key you now hold a
`MethodNode[]` *plus* a `HashMap` *plus* a list per distinct name — across
hundreds of keys, and multiplied per `ClassLoader` in app-server-style setups.
Probably a few hundred KB and clearly worth it; I'd just want one sentence
stating it.*
**Quantification:**
- Across the standard Groovy GDK/extension library, there are approximately
~1,200 extension methods spread across ~180 distinct receiver types (such as
`Object`, `Collection`, `List`, `Map`, `String`, arrays).
- Each receiver key maintains a `MethodsByName` instance with an
appropriately pre-sized `HashMap` (using initial capacity `Math.max(4,
(int)(count / 0.75f) + 1)` and `Collections.singletonList` for single-method
buckets).
- Across all ~180 receiver types in a `ClassLoader`, the total retained
memory for the name index structures is approximately **150 KB to 250 KB per
`ClassLoader`**.
- This small, fixed retained size is negligible in relation to typical
classloader AST/bytecode footprints, while eliminating linear scans over
hundreds of methods on root types.
---
### 3. Benchmark Interpretation
> **Review Comment:**
> *The write-up overstates what the data shows... I'd trim the claims to
what the data supports, and strip the
`file:///home/daniel/IdeaProjects/groovy/...` links before this lands in an ASF
commit record.*
**Clarification:**
- **Allocation Reduction:** The primary optimization in `chooseBestMethod`
is the exact elimination of temporary `Parameter[]` allocations on non-generic
method dispatch paths.
- **Lookup Microbenchmark:** `DgmMethodLookupBench` confirms algorithmic
$O(1)$ speedup (3× to 10× faster) on high-traffic receiver types (`Object`,
`List`, `String`, arrays).
- **Macro/Compilation Benchmark:** Acknowledged that macro/compile time
deltas have overlapping confidence intervals on synthetic workloads; the PR
description and commit notes focus on the verified allocation reduction and
algorithmic lookup improvements.
> STC: index extension methods by name and skip cloning non-generic parameters
> ----------------------------------------------------------------------------
>
> Key: GROOVY-12285
> URL: https://issues.apache.org/jira/browse/GROOVY-12285
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
>
> The static type checker resolves DGM (Default Groovy Methods) and other
> extension methods by walking the receiver hierarchy and collecting methods of
> a given name. {{ExtensionMethodCache}} stores a flat list per receiver type,
> so each named lookup scans every method on that type. Receivers such as
> {{Object}} and {{Collection}} have hundreds of DGM methods, and that scan
> sits on the compile hot path.
> {{chooseBestMethod}} erases generic parameter types before measuring
> argument-parameter distance. It currently clones every candidate's parameter
> array to do so, including methods that have no generic parameters.
> h3. Proposed change
> * When a class loader's extension methods are scanned, index each receiver
> list by method name so a named lookup is a hash get rather than a linear scan.
> * Clone a candidate's parameter array only when at least one parameter is a
> generics placeholder or otherwise uses generics.
> * Drop derived indexes together with the loader's method map so they cannot
> go stale independently.
> {code:java}
> // today
> for (MethodNode node : fromDGM) {
> if (node.getName().equals(name)) accumulator.add(node);
> }
> // proposed
> accumulator.addAll(EXTENSION_METHOD_CACHE.get(loader, className, name));
> {code}
> h3. Impact
> Compile-time only. Named lookup results and overload selection stay the same.
> {{MethodNode}} parameter arrays are not mutated.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
