paulk-asert commented on PR #2645:
URL: https://github.com/apache/groovy/pull/2645#issuecomment-5086680305
## Follow-up (a) in more detail: preemption-check cost — benchmark plan +
proposed gate
Expanding on the compile-time concern noted above, since it's the one open
item that could block merge.
### The cost, precisely
The earlier allowlist design had an accidental virtue:
`classTagPreemptionTargets.contains(name)` was an O(1) pre-gate that was false
for virtually every call, so resolved calls paid nothing. In the revised
design, `isPreemptionPossible()` is still O(1) but almost always *true*, after
which `matchPreemption` runs for every resolved method call under
`@TypeChecked`/`@CompileStatic`:
- an incumbent-kind scan over the matched methods (cheap, usually one
element);
- for extension-method incumbents — the dominant case (`each`, `collect`, …)
— a `findDGMMethodsForClassNode` lookup, which is cache-backed but still walks
the receiver hierarchy and filters by name per call;
- intent/veto checks that call `getAnnotations(...)` per parameter per
candidate.
Almost always the outcome is "no preempt-intent overload exists here" — we
pay enumeration cost to discover a negative that is knowable in advance. That's
the target.
### Proposed gate: a per-classloader "preemptive names" set in
`ExtensionMethodCache`
`ExtensionMethodCache` already solves the exact lifecycle problem we need:
it caches the receiver→methods map per classloader and invalidates when
extension modules change. The plan piggybacks on the same scan:
- Change the cached value to a small holder — `{byReceiverType map;
preemptiveNames set}` — so the name set shares the map's lifecycle and can
never go stale independently (no separate invalidation logic to get wrong).
- While `AbstractExtensionMethodCache` accumulates each `MethodNode`,
additionally record the method name if any parameter carries
`@ClassTag(preempt=true)`. One-time cost, amortized across the compilation.
- Fast path in `matchPreemption`: when all incumbents are extension methods
and `!preemptiveNames.contains(name)`, return before any candidate enumeration.
Out of the box that set contains exactly `{"withDefault"}`, so ~every DGM call
short-circuits on one set lookup — restoring the allowlist's O(1) profile while
keeping author self-declaration.
Two constraints worth stating: the gate must be *conservative* (false
positives fall through to the full match, which fails softly; false negatives
would be a correctness bug — hence deriving the set from the same scan that
feeds method lookup, never a hand-maintained list), and it only covers the
extension-incumbent path. Instance-method incumbents still scan the receiver's
same-name overloads — bounded, and the rarer path. If measurement says that
matters, the cheap fix is reordering `matchOverload` to compare arity (integer
math) before scanning tag annotations (list allocation), so untagged overloads
bail for free; I'd hold that until the data asks for it.
### Benchmark plan
Compile-time benchmarks are noisy, so: JMH, fresh
`GroovyClassLoader`/`CompilationUnit` per invocation, compiling to
`INSTRUCTION_SELECTION` (captures all STC work, skips classgen noise),
average-time mode with proper warmup rather than single-shot.
**Corpus**: a generated synthetic source set (reproducible, tunable) — ~200
`@CompileStatic` classes × 50 calls, in three mixes as benchmark params:
- `dgmHeavy` — DGM calls on typed receivers (the path the gate targets);
- `instanceHeavy` — same-class/hierarchy instance calls with overloads (the
path it doesn't);
- `withDefault` — calls that actually preempt, confirming the happy path
doesn't regress.
**Variants**: (1) master baseline; (2) this branch as-is; (3) branch with
`classTagPreemptionDisabled = ['*']` — the floor the gate could reach; (4)
branch + name-set gate.
**Acceptance criteria**: if (2) is within ~1–2% of (1) on `dgmHeavy`, ship
as-is and the gate becomes optional hygiene; otherwise land the gate and expect
(4) ≈ (3) ≈ (1). A coarse cross-check against the existing `:performance`
corpus is easy to add, but the variant comparison wants the JMH harness.
**Scope note**: I don't propose extending the gate to instance methods via
per-`ClassNode` cached flags — the invalidation story during compilation
(classes still being built, AST mutation) is genuinely tricky, and the bounded
per-call scan is very likely in the noise. The benchmark exists to prove or
refute that cheaply before adding machinery.
I'll post numbers here once the harness runs; if anyone has a preferred
real-world `@CompileStatic`-heavy corpus to include alongside the synthetic
one, suggestions welcome.
--
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]
