[
https://issues.apache.org/jira/browse/GROOVY-12285?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18107021#comment-18107021
]
ASF GitHub Bot commented on GROOVY-12285:
-----------------------------------------
paulk-asert commented on PR #2823:
URL: https://github.com/apache/groovy/pull/2823#issuecomment-5383452701
AI read:
> **Verdict: technically sound, low risk, mergeable. Two things I'd want
changed first (both small), and the PR description needs trimming before it
becomes the commit/JIRA record.**
>
> ## What it actually changes
>
> Two independent optimisations plus a doc line:
>
> 1. **Name-indexed extension-method cache.** `makeMethodsUnmodifiable` now
wraps each per-receiver list in a private `MethodsByName` (an immutable
`AbstractList` + `RandomAccess` carrying a `HashMap<String,
List<MethodNode>>`). `findDGMMethodsForClassNode` replaces its linear
`node.getName().equals(name)` scan with a hash lookup.
> 2. **Lazy parameter erasure.** `chooseBestMethods` no longer
unconditionally clones `Parameter[]`; the new `parametersForDistance` clones
only on first generic/placeholder parameter and otherwise returns the original
array.
>
> ## Correctness — checked, not assumed
>
> - **The name index is behaviour-identical.** The index key is
`method.getName()`, the same predicate the old loop used, and the accumulator
is still the same `TreeSet` with `DGM_METHOD_NODE_COMPARATOR`, so ordering and
dedup are unchanged. The new test `'name index matches a linear scan of the
cached list'` asserts exactly this equivalence.
> - **The aliasing question resolves clean.** I traced every consumer of the
array returned by `parametersForDistance` —
`measureParametersAndArgumentsDistance` and its callees
(`allParametersAndArgumentsMatch`, `firstParametersAndArgumentsMatch`,
`lastArgMatchesVarg`, `excessArgumentsMatchesVargsParameter`,
`getVarargsDistance`, `isVargs`). All are read-only;
`firstParametersAndArgumentsMatch` copies into a fresh array rather than
writing in place. So handing out `MethodNode`'s internal parameter array is
safe as the code stands.
> - **Immutability is preserved.** `AbstractList` throws
`UnsupportedOperationException` for `add`/`set`/`remove`/`clear`/`sort` without
overrides, so `MethodsByName` is as immutable as the
`Collections.unmodifiableList` it replaces. The tests cover this.
> - **Bonus: it fixes a real latent bug.** On master,
`clearExtensionMethodCache(loader)` does
`EXTENSION_METHOD_CACHE.cache.remove(loader)`
(`StaticTypeCheckingSupport.java:279`) and never touches `preemptiveNamesCache`
— despite the javadoc claiming the preempt set "shar[es] its loader lifecycle,
so it never goes stale independently." That was untrue: the GROOVY-12115
preempt-name set could survive a cache clear and go stale. The new
`invalidate`/`invalidateAll` drop both, and there's a test for it. Worth
calling out in the JIRA, because it's a correctness fix hiding inside a perf PR.
>
> **CI is fully green** — JDK 17/21/25, Linux/macOS/Windows, all
`additional` module shards, CodeQL, SonarCloud, and both JMH suites.
>
> ## Two changes I'd ask for
>
> **(a) The downcast is brittle.** 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.
>
> **(b) The lost `clone()` was an accidental safety barrier.**
`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.
>
> ## Smaller notes (not blockers)
>
> - **The macro cache pays for an index it never uses.**
`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.
> - **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.
> - **The write-up overstates what the data shows.** By the PR's own
numbers, `ChooseBestMethodBench` is *slower* in ns/op in 3 of 5 scenarios
(926→972, 2303→2336, 1394→1466), with error bars far larger than the deltas —
time is a wash there, and the defensible claim is the allocation reduction
(which is exact and well argued). `StcCompilePerfBench` reports 416.65 ± 87.58
vs 336.28 ± 56.97 — those intervals overlap substantially, so "1.24× speedup /
−19.3%" is not established at the stated 99.9% confidence. The lookup
microbenchmark speedups (3–10×) are large enough relative to their error to be
real. 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.
> 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)
