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 |
> | ------------------------ | ------------------- | ------------------- |
----------- |
> | `largeScaleClass` | 724.88 **± 304.30** | 573.24 **± 209.59** |
+20.92% |
> | `deepHierarchy` | 102.64 ± 34.32 | 91.21 ± 28.93 |
+11.13% |
> | `polymorphicCollections` | 90.89 ± 38.35 | 85.08 ± 23.81 |
+6.40% |
> | `mergeHeavy` | 178.80 ± 55.14 | 170.63 ± 50.88 |
+4.57% |
> | `staticCompileMerge` | 188.25 ± 38.36 | 213.55 ± 31.61 |
**−13.44%** |
> | `deeplyNestedBranches` | 156.94 ± 26.41 | 162.33 ± 35.01 |
**−3.44%** |
> | `exceptionHierarchy` | 190.85 ± 41.01 | 199.75 ± 48.42 |
**−4.66%** |
>
> Four faster, three slower, **every single confidence interval
overlapping**. The headline result carries an error bar of ±42% of its own mean
— [420, 1029] vs [364, 783]. That is a coin flip, not a measurement. Promoting
"+20.92% faster total compilation" out of that table into the executive summary
isn't supportable.
>
> Two further problems:
>
> - **The allocation claim is contradicted by the PR's own numbers.**
Alloc-per-op is essentially unchanged in every scenario: 395.20 → 394.40 MB,
59.83 → 59.81, 106.74 → 106.75, 131.84 → 131.95, 130.38 → 130.45. The
eliminated `replace('/','.')` strings produce **no measurable allocation
reduction**. Yet the summary claims "−27.38% GC pause time" — while the alloc
*rate* for that same scenario went **up** (584 → 721 MB/s), and GC time went up
in three other scenarios (+7.06%, +7.66%, +1.33%). GC pause total is a function
of run duration and heap state, not a controlled metric here; that number is
noise, cherry-picked.
> - **The warmup is too short to converge.** 3 warmup iterations × 2s on a
workload running 100–700 ms/op gives roughly 3–20 executions of the whole
Groovy compiler per warmup iteration — nowhere near JIT steady state. The ±40%
error bars are the symptom.
> - **The "Isolated Phase 7" table has no error bars and no stated
methodology** — it isn't produced by the benchmark file in this PR, so as
presented it's unreproducible.
>
> Same housekeeping point as 2823: this text becomes the commit message and
JIRA record. The honest version is *"removes redundant hierarchy traversal at
frame-merge points; microbenchmark shows the effect on deep-hierarchy
workloads, whole-compilation effect is within noise."* That's still a perfectly
good reason to merge.
>
> ## Benchmark plumbing
>
> `ClassWriterBytecodeGenBench` matches `\.perf\.[A-G]`, so it gets
**auto-enrolled into the `core-ag` daily suite and run twice — indy and
classic.** As with 2823, the indy/classic split measures nothing for a
compile-time benchmark; that's half the cost for zero signal. Runtime fits
comfortably (core-ag came in at 35 min against a 90-min timeout on this PR).
The more consequential issue: publishing a benchmark whose error bars are ±40%
adds a permanently noisy series to the daily dashboard that will trip the alert
threshold at random.
>
> ## Connection to the compiler dashboard question
>
> Worth noting given the earlier discussion: **unlike 2823, this change
*does* affect the published compiler performance chart.** `CLASS_GENERATION`
runs for all Groovy code, dynamic included, and the PLEAC corpus hits
`getCommonSuperClass` at every branch merge. So `compile@current` at
`dev/bench/perf/compiler/` should move if the effect is real.
>
> That's a free, well-controlled validation the JMH run can't give you: 50
warmup + 300 measured full-corpus compiles, on stable hardware, against three
fixed released-version baselines on the same chart. If the daily run shows no
movement in `compile@current` while `compile@groovy-3/4/5` hold steady, that
settles whether the JMH numbers were signal or noise — far more convincingly
than re-running the microbenchmark.
>
> ## Recommendation
>
> Merge the code once the symmetry test and comment are added. Ask for the
description to be rewritten to the claims the data actually supports before it
lands as the commit record, and drop the GC-pause and
total-compilation-throughput headlines. Separately, raise the
CompilationUnit-scoped-cache question — that's a plausible follow-up with a
much better return than what's here.
--
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]
