GROOVY-12162: Harden and extend bytecode peephole optimization

[daniellansun]

https://issues.apache.org/jira/browse/GROOVY-12162

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.

Benchmark suite	Current: cc7a492	Previous: c76e8dc	Ratio
org.apache.groovy.bench.StaticMethodCallIndyBench.staticChain_groovyCS	2523.685772398189 ops/ms	1595.6378191431031 ops/ms	1.58
org.apache.groovy.bench.StaticMethodCallIndyBench.staticChain_java	2520.3046195893407 ops/ms	1595.5729626739744 ops/ms	1.58
org.apache.groovy.bench.AryBench.java ( {"n":"1000"} )	0.1193460639887132 ms/op	0.056632317821343335 ms/op	2.11
org.apache.groovy.bench.CalibrationBench.memoryPointerChase	993.5820446000793 us/op	507.29968992095434 us/op	1.96

This comment was automatically generated by workflow using github-action-benchmark.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 78.20513% with 102 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.2369%. Comparing base (3740ee7) to head (cc7a492).
⚠️ Report is 26 commits behind head on master.

Files with missing lines	Patch %	Lines
.../classgen/asm/PeepholeOptimizingMethodVisitor.java	78.8764%	36 Missing and 58 partials ⚠️
...rg/codehaus/groovy/classgen/AsmClassGenerator.java	58.3333%	5 Missing ⚠️
...codehaus/groovy/classgen/asm/WriterController.java	50.0000%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##               master      #2707        +/-   ##
==================================================
+ Coverage     69.1120%   69.2369%   +0.1249%     
- Complexity      34251      34685       +434     
==================================================
  Files            1537       1540         +3     
  Lines          129403     130312       +909     
  Branches        23526      23755       +229     
==================================================
+ Hits            89433      90224       +791     
- Misses          31944      31980        +36     
- Partials         8026       8108        +82     

Files with missing lines	Coverage Δ
...g/codehaus/groovy/classgen/asm/CallSiteWriter.java	85.5769% <100.0000%> (-0.1374%)	⬇️
...y/classgen/asm/PeepholeOptimizingClassVisitor.java	100.0000% <100.0000%> (ø)
...codehaus/groovy/classgen/asm/WriterController.java	86.1111% <50.0000%> (-1.1229%)	⬇️
...rg/codehaus/groovy/classgen/AsmClassGenerator.java	85.0399% <58.3333%> (-0.0217%)	⬇️
.../classgen/asm/PeepholeOptimizingMethodVisitor.java	86.1219% <78.8764%> (-8.5305%)	⬇️

... and 42 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

JMH summary — classic (commit 3381530)

Speedup vs trailing 90-day baseline on gh-pages. Higher = faster.
1.00 = in line with history. Per-benchmark ratio, geomean within group.
Time-per-op units inverted so direction is consistent. The calibrated
column divides out this runner's speed vs the baseline hardware, as
measured by Groovy-independent pure-Java ruler benchmarks.

Group	Speedup	Calibrated	n
bench	0.972 ×	1.025 ×	84
core	1.043 ×	1.013 ×	77
grails	1.017 ×	1.045 ×	80

_{Runner calibration (this run vs baseline hardware): bench 0.96× (26 rulers) · core-ag 1.08× (3 rulers) · core-hz 0.98× (3 rulers) · grails-ad 0.96× (3 rulers) · grails-ez 0.98× (3 rulers)}

_{Baseline: dev/bench/jmh/<part>/classic/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data}

[github-actions]

JMH summary — indy (commit 3381530)

Speedup vs trailing 90-day baseline on gh-pages. Higher = faster.
1.00 = in line with history. Per-benchmark ratio, geomean within group.
Time-per-op units inverted so direction is consistent. The calibrated
column divides out this runner's speed vs the baseline hardware, as
measured by Groovy-independent pure-Java ruler benchmarks.

Group	Speedup	Calibrated	n
bench	0.985 ×	0.991 ×	84
core	4.283 ×	3.763 ×	77
grails	2.199 ×	2.313 ×	80

⚠️ Runner speed differs ≥15% from the historical baseline hardware for: core-hz. Raw speedups are not meaningful for those parts — use the calibrated column.

_{Runner calibration (this run vs baseline hardware): bench 0.99× (26 rulers) · core-ag 1.09× (3 rulers) · core-hz 1.20× (3 rulers) · grails-ad 0.97× (3 rulers) · grails-ez 0.93× (3 rulers)}

_{Baseline: dev/bench/jmh/<part>/indy/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data}

[daniellansun]

If no one objects to this PR, I'll merge it this weekend.

[paulk-asert]

AI read:

Verdict: sound, well-engineered, and ship-able — with one design inconsistency worth reconciling before merge.

The one issue to reconcile — discarded-CHECKCAST CCE is treated inconsistently.

Within this same PR, a cast whose result is discarded is handled two opposite ways:

• before POP (tryRemovePendingLoad): the cast is dropped — "drop the dead cast, keep the pop." The PR extends this so even ALOAD; CHECKCAST; POP collapses to nothing (test removesLoadAndAttachedCheckcastBeforePop asserts → ['RETURN']). → the ClassCastException is eliminated.
• before void RETURN (tryDropPendingLoadOnReturn): the cast is preserved — emits CHECKCAST; POP; RETURN "so a type-check side effect is preserved." → the CCE is kept.

Same situation (cast value thrown away), opposite decisions based only on POP vs RETURN. By the author's own stated principle (preserve the type-check side effect), the POP path is the inconsistent one. Impact is likely low — the standalone drop shipped back in GROOVY-12065, the full CI matrix is green, and Groovy rarely emits a bare CHECKCAST; POP for a discarded cast (dynamic casts go through castToType) — so I'd rate it PLAUSIBLE, not a confirmed regression. But it's a real semantic question worth settling: either a discarded cast's CCE matters (then preserve it on the POP path too) or it doesn't (then simplify the RETURN path to drop it). A behavior test pinning a discarded failing cast under @CompileStatic would lock the intended semantics.

[paulk-asert]

LGTM but change if possible assuming the Claude read holds per your analysis.

[blackdrag]

actually we could compress this to ILOAD0; I2L; LRETURN.

[daniellansun]

I’d rather not. Integer.valueOf + Long.longValue must fail at runtime on the
wrong receiver (CCE / ICCE path). Rewriting to I2L would silently change
semantics. Mismatched unbox stays intact on purpose.

[blackdrag]

I was going by the test, and here no wrong receiver is possible. You cannot get a CCE from an int using valueOf and not from longValue from the resulting instance. Though I wonder in what situation we would even emit that kind of code. For long l = 1? I think that would directly use I2L.

[daniellansun]

You’re right — thanks for pushing on this.

With a pending Integer.valueOf the receiver is always Integer, so there is
no data-dependent “wrong receiver” CCE path worth preserving. Real classgen
already uses I2L for long l = intExpr; the box+Long.longValue form in the
test was artificial.

Done: pending box(P) followed by unbox-as-Q now rewrites to the matching
JVM conversion (I2L / L2I / I2F / …), including cross-wrapper cases such as
Integer.valueOf + Long.longValue. Same-type cancel is unchanged; Boolean is
still excluded from numeric conversion.

[daniellansun]

AI read:

Verdict: sound, well-engineered, and ship-able — with one design inconsistency worth reconciling before merge.
The one issue to reconcile — discarded-CHECKCAST CCE is treated inconsistently.
Within this same PR, a cast whose result is discarded is handled two opposite ways:

• before POP (tryRemovePendingLoad): the cast is dropped — "drop the dead cast, keep the pop." The PR extends this so even ALOAD; CHECKCAST; POP collapses to nothing (test removesLoadAndAttachedCheckcastBeforePop asserts → ['RETURN']). → the ClassCastException is eliminated.
• before void RETURN (tryDropPendingLoadOnReturn): the cast is preserved — emits CHECKCAST; POP; RETURN "so a type-check side effect is preserved." → the CCE is kept.

Same situation (cast value thrown away), opposite decisions based only on POP vs RETURN. By the author's own stated principle (preserve the type-check side effect), the POP path is the inconsistent one. Impact is likely low — the standalone drop shipped back in GROOVY-12065, the full CI matrix is green, and Groovy rarely emits a bare CHECKCAST; POP for a discarded cast (dynamic casts go through castToType) — so I'd rate it PLAUSIBLE, not a confirmed regression. But it's a real semantic question worth settling: either a discarded cast's CCE matters (then preserve it on the POP path too) or it doesn't (then simplify the RETURN path to drop it). A behavior test pinning a discarded failing cast under @CompileStatic would lock the intended semantics.

Agreed — same situation, opposite decisions. We settled on preserve CCE on both paths (Java-like; matches the void-RETURN intent).

Change: tryRemovePendingLoad now emits attached/standalone CHECKCAST before POP instead of dropping it. Pure loads without a cast still collapse.

Tests:

• unit: POP vs RETURN both keep CHECKCAST (preservesLoadAndAttachedCheckcastBeforePop, preservesStandaloneCheckcastBeforeReturn, …)
• SC: Groovy12162 — discarded Groovy cast keeps side effect; after instanceof String, discarded (String) o still emits CHECKCAST; POP (not erased)

[daniellansun]

@blackdrag @paulk-asert
Thanks for your reviewing.
The PR has been updated, please review it again.

[sonarqubecloud]

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
85.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: cc7a492
▶️ Tests: 105506 executed
⚪️ Checks: 31/31 completed

---

Learn more about TestLens at testlens.app.