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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
