[
https://issues.apache.org/jira/browse/GROOVY-12288?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18107027#comment-18107027
]
ASF GitHub Bot commented on GROOVY-12288:
-----------------------------------------
paulk-asert commented on PR #2825:
URL: https://github.com/apache/groovy/pull/2825#issuecomment-5383749272
AI thoughts below. I haven't tried to run locally yet, so I'm not sure how
to read the mixed perf data yet.
> I've reviewed the diff against master, verified the scoping and caching
assumptions in the actual source, and checked CI.
>
> ---
>
> # PR 2825 (GROOVY-12288) — merge readiness
>
> **Verdict: the code is correct and low-risk — arguably lower-risk than
2823 — and CI is fully green. But the performance case is not supported by the
data presented, and one of the two caches largely duplicates memoization that
already exists. I'd merge the code; I'd reject the PR description as written.**
>
> ## What it changes
>
> `CompilationUnit.createClassVisitor()` replaces the anonymous
`ClassWriter` with a private inner `CachingClassWriter` carrying two maps:
> - `classNodeByInternalName` — internal name → resolved `ClassNode`
> - `commonSuperByPair` — memoized `getCommonSuperClass` results, stored
under both `(A,B)` and `(B,A)`
>
> plus identity fast-paths (`commonNode == class1` → return `type1`
verbatim) and a constant for `java/lang/Object`.
>
> ## Correctness — verified
>
> - **Scoping is exactly as claimed.** `createClassVisitor()` is called once
per generated class inside the `classgen` operation
(`CompilationUnit.java:860`), and again recursively for each inner class. Both
maps are born and die with one class. No leak, bounded memory.
> - **Negative results are deliberately not cached** (`if (cn != null)
classNodeByInternalName.put(...)`). This is the right guard and it matters:
closure/inner classes are registered *during* class generation, so a name
unresolvable at one moment may resolve later. Caching a null would have been a
real bug. It's handled.
> - **`getClassNode` restructuring is behaviour-preserving** — the
early-return chain became nested `if`s with identical ordering (`cu.getClass` →
`getGeneratedInnerClass` → `ClassNodeResolver`).
> - **The failure mode improves.** Previously an unresolvable name produced
an NPE inside `getCommonSuperClassNode`; now it throws `GroovyBugError("Unable
to determine common super class of X and Y")`. `GroovyBugError` is already
imported (`CompilationUnit.java:24`) and this matches house style elsewhere in
the file (lines 992, 1050 wrap NPEs the same way). It *is* an observable change
— worth a line in the JIRA rather than leaving it silent.
> - **The risk profile is inherently benign.** This computes StackMapTable
frames. If it were wrong you'd get a `VerifyError` at class load — loud,
immediate, impossible to miss — not silent misbehaviour. Every class the Groovy
test suite compiles exercises this path, and the full matrix (JDK 17/21/25,
Linux/macOS/Windows, all `additional` shards, `dist`, all JMH suites) is green.
> - **Test shape is right.** The four new tests compile, `defineClass`, and
then *invoke* the results, so the JVM verifier itself validates the frames.
That's much better than asserting on internals.
>
> ## The one invariant I'd want covered
>
> Storing each result under both `(A,B)` and `(B,A)` **newly asserts that
`getCommonSuperClassNode` is commutative.** Reading it, I believe it is: the
loop walks `c`'s superclass chain to the lowest ancestor that `d` derives from
— the LCA either way under single inheritance — and the interface branch is
symmetric. But this is a new invariant the code now depends on, and nothing
tests or documents it. There's no test asserting `f(A,B) == f(B,A)`. I'd ask
for one test plus a one-line comment stating the assumption. Cheap, and it
protects the invariant against future edits to that walk.
>
> Secondary, smaller: the identity fast-path returns `type1`/`type2`
verbatim rather than `commonNode.getName().replace('.','/')`. These should be
identical since the node was resolved *from* that string, and returning ASM's
own string is arguably safer — but it's a silent difference if a resolver ever
returns a node whose name differs from the name requested. One sentence of
justification in the comment would settle it.
>
> ## The substantive design critique
>
> **`classNodeByInternalName` mostly duplicates a cache that already
exists.** `ClassNodeResolver` maintains its own `cachedClasses` HashMap —
*including a negative cache* (`NO_CLASS`) — and the resolver is a
per-`CompilationUnit` field (`CompilationUnit.java:120`). So the expensive path
(ClassLoader / classpath lookup) was **already memoized across the entire
compilation** before this PR.
>
> That means the new map does not eliminate "ClassLoader access and
classpath lookups" as the description claims. It eliminates one
`replace('/','.')` allocation, two map lookups, and one *already-cached*
resolver hit. Real, but small — and this is corroborated by the PR's own
allocation figures, which I'll come to.
>
> The genuine win is `commonSuperByPair`, which skips the `isDerivedFrom`
hierarchy walk. That's the actual O(depth) work and the only mechanism that
plausibly explains the `deepHierarchy` result.
>
> **And the design leaves the larger win on the table.** Because the cache
dies with each class, a project of many small classes gets near-zero reuse —
the pair cache is rebuilt from scratch for every class, even though the common
pairs (`String`/`Object`, collection types) recur across *all* of them. A
CompilationUnit-scoped pair cache would capture that. The obvious objection is
staleness, but the code already handles the only real hazard by not caching
negatives, and hierarchy links are fixed well before `CLASS_GENERATION`. If
Daniel considered CU scoping and rejected it, the PR should say why; if not,
that's where the actual performance is.
>
> ## The performance claims don't survive contact with the data
>
> This is my main objection, and it's the same pattern as 2823 but
considerably worse.
>
> Read the PR's own table 5.1:
>
> | Scenario | Baseline | Optimized |
Claimed |
> | ----------------------
> Cache ClassWriter getCommonSuperClass lookups per class
> -------------------------------------------------------
>
> Key: GROOVY-12288
> URL: https://issues.apache.org/jira/browse/GROOVY-12288
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
>
> Bytecode generation uses an ASM {{ClassWriter}} with {{{}COMPUTE_FRAMES{}}}.
> Frame computation calls {{getCommonSuperClass}} at every control-flow merge.
> Groovy overrides that method so types still being compiled are resolved
> through {{ClassNode}} ({{{}CompileUnit{}}}, generated inner classes,
> {{{}ClassNodeResolver{}}}) rather than {{{}Class.forName{}}}.
> {{COMPUTE_FRAMES}} asks for the same binary-name pairs many times inside one
> class. Each call converts slashes to dots, resolves two \{{ClassNode}}s, and
> walks superclasses with isDerivedFrom. Class generation is about half of
> compile wall time.
> h3. Approach
> Memoize both steps on the {{ClassWriter}} created by
> {{{}CompilationUnit.createClassVisitor{}}}. One writer is allocated per
> generated class and discarded afterwards, so the maps cannot go stale across
> classes.
> ||Cache||Key||Value||
> |{{classNodeByName}}|binary name (dot form)|{{ClassNode}} (successful lookups
> only)|
> |{{commonSuperByPair}}|canonical pair of internal names|internal name of the
> common superclass|
> The key is order-independent: {{(A,B)}} and {{(B,A)}} share one entry. The
> common-superclass algorithm is unchanged.
> h3. Impact
> Compile-time only. {{getCommonSuperClass}} results and generated bytecode
> stay the same.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
