daniellansun commented on PR #2852: URL: https://github.com/apache/groovy/pull/2852#issuecomment-5510989609
@blackdrag Thanks for the review — the hidden-class encoding staying acceptable is a relief. The complexity-vs-gain concern is fair, and the benches were incomplete in the way you described. I have pushed follow-ups and a short measurement note (`jira-drafts/pr-2852-performance-report.md` locally; I can paste numbers here). **Baseline.** `startsWith_java` is now labelled a lower bound only. The baseline is reflective `CachedMethod.invoke` (`-Dgroovy.cachedmethod.invoker.disable=true`). Operands are `@Param` fields so the JIT cannot fold `startsWith` to a constant. Against `03e303e` on JDK 25 (JMH 1.37, 1 fork, 3×1 s / 5×1 s): | Path | ns/op | |---|---| | `03e303e` `CachedMethod.invoke` | 18.0 ± 2.7 | | HEAD, generation disabled | 17.8 ± 1.3 | | HEAD, generated (warmed, `threshold=0`) | 10.1 ± 2.5 | | Java `startsWith` | 7.3 ± 0.8 | So the interned-MOP case is about **1.8×** the pre-change path, and the kill-switch path is not a regression within error. **Create cost / break-even.** New rows use a fresh `CachedMethod` per invocation: - `generate_reflective_fresh` ≈ 0.09 µs - `generate_then_invoke` (`threshold=0`) ≈ 77 µs Almost all of that is `defineHiddenClass`. At ~8 ns saved per later invoke, naïve break-even is on the order of 10⁴ calls **after** generation. That cost is paid **once per interned `CachedMethod` per VM**, which is the real MOP (`CachedClass` method table). A throwaway `CachedMethod` used a few hundred times does not recoup — the `burst_*` rows show that on purpose (250 calls with default threshold ≈ 104 µs vs 4 µs staying reflective, because the burst includes one define). Default threshold 100 is unchanged: below `groovy.indy.optimize.threshold` (1000), so methods that never look hot never define a class. The first disable-path run was ~31 ns vs 18 ns on `03e303e` because `generationAllowed()` re-read system properties on every invoke. That is now sticky on the instance when generation is off (kill switch / native-image / Android). The threshold is cached on the instance so lukewarm invokes do not call `Long.getLong` every time. **Why `CachedMethod`.** This is not a third invoke engine beside indy and `CallSiteGenerator`. Those already have a JIT-constant target (linked `CallSite` / emitted `INVOKE*`). `CachedMethod.invoke` is the leftover `Method.invoke` on the MOP “Groovy as caller” path (including cold indy still on `doMethodInvoke`). The trampoline is bound to that `Method` and must not be fed to indy (wrong `Lookup`). Putting the hook on `CachedMethod` is the smallest place that sees every such invoke without widening public API. I kept the fast path to a volatile read + generate-or-reflect; factory/ASM live under `org.apache.groovy.internal.runtime.invoke`. Happy to drop the default threshold, or the whole hook behind `disable=true` by default, if you would rather ship the machinery dark until we have a longer-lived application bench. -- 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]
