[
https://issues.apache.org/jira/browse/GROOVY-12259?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18105041#comment-18105041
]
ASF GitHub Bot commented on GROOVY-12259:
-----------------------------------------
blackdrag commented on code in PR #2786:
URL: https://github.com/apache/groovy/pull/2786#discussion_r3790751828
##########
src/main/java/org/apache/groovy/runtime/indy/IndyInvalidation.java:
##########
@@ -180,15 +180,23 @@ public static void invalidateUnscoped() {
}
/**
- * Detaches and invalidates every live class-level MetaClass / pending
domain
- * for loaded types. Shared implementation for all process-wide entries.
+ * Detaches and invalidates every live MetaClass / pending domain via the
+ * live-SwitchPoint registry — O(live domains), replacing the former walk
+ * over {@link ClassInfo#getAllClassInfo()} which cost O(loaded classes)
+ * twice per category {@code use} block (GROOVY-12259). Shared
+ * implementation for all process-wide entries.
+ * <p>
Review Comment:
As I said to Daniel in the other PR, this is very much AI generated work
report content. I don´t think it should be in the javadoc. This PR changes what
the class does, it is not the place of the javadoc to mention what it did
before, unless for compatibility, but compatibility is not the focus here.
Everything after live domains and before Shared should be removed
> Make category/bulk call-site invalidation O(live SwitchPoint domains) instead
> of O(loaded classes)
> --------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12259
> URL: https://issues.apache.org/jira/browse/GROOVY-12259
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
>
> GROOVY-12258 made process-wide call-site invalidation a no-op in processes
> that never link an indy MOP guard (classic-only bytecode). This follow-up
> addresses the remaining cost for indy and mixed processes: once any indy site
> has linked, every category enter/leave (and custom-MetaClass / unattributed
> registry event) still retires domains by walking every loaded class.
> h3. Problem
> {{IndyInvalidation.retireAllLoadedDomains()}} iterates
> {{ClassInfo.getAllClassInfo()}} — O(all loaded classes), a weak-reference
> pointer chase over tens of thousands of entries in a framework-sized app —
> *twice per {{use}} block* (enter and leave), collecting the comparatively few
> domains that actually hold a live SwitchPoint.
> h3. Proposed fix
> Track live SwitchPoints in a process-wide registry so bulk retirement is
> O(live domains):
> * {{SwitchPointInvalidator}} gains a static {{ConcurrentHashMap<SwitchPoint,
> SwitchPointInvalidator>}} of every live SwitchPoint mapped to its owning
> invalidator. It is keyed by *SwitchPoint*, not invalidator: each SwitchPoint
> has a single-use lifecycle (allocated once, detached once), so a removal can
> never clobber a successor's entry the way an invalidator-keyed registry could
> (ABA on re-allocation).
> * {{getSwitchPoint()}} registers the new SwitchPoint *before* the publishing
> CAS (and deregisters on CAS loss). The GROOVY-12258 ordering argument carries
> over: a bulk path that finds no entry is guaranteed that SwitchPoint was not
> yet visible to any guard, so skipping it is safe.
> * Both detach paths ({{detachLive()}} and the new {{detachIfCurrent(sp)}})
> deregister on successful detach, so all maintenance funnels through the
> existing allocation/retirement choke points; per-class invalidation needs no
> changes.
> * {{retireAllLoadedDomains()}} drains the registry: each entry is claimed via
> {{detachIfCurrent}}, and an entry is removed *only on a successful claim*.
> Removing on a failed claim would strand a concurrently-publishing SwitchPoint
> permanently invisible to all future drains (a correctness trap: pre-publish
> entries must survive the drain); failed-claim entries are transient and are
> cleaned up by their owner.
> * The GROOVY-12258 monotonic flag is replaced by {{registry-is-empty}}, which
> is strictly stronger: it also skips after all domains have retired, and
> re-arms rather than being one-shot. The classic-only skip is preserved.
> The drain's weakly consistent iteration can miss a SwitchPoint published
> mid-drain — the same window as the previous all-classes walk; sites linking
> concurrently read the current category state at link time, unchanged. One
> trade-off to note: the registry holds keys strongly, so a domain whose
> MetaClass has died stays pinned (one small SwitchPoint + invalidator) until
> any bulk event sweeps it.
> h3. Measurements
> JMH ({{-PbenchInclude=CategoryBench.categoryInLoop :perf:jmh}}, 2 forks x 5
> iterations, JDK 21, same machine throughout):
> ||build||indy||classic||
> |Groovy 5 (5.1.x branch)|1489.8 ± 39.1 ms/op|163.2 ± 12.8 ms/op|
> |Groovy 6 pre-GROOVY-12191|1269.1 ± 45.5 ms/op|147.5 ± 12.0 ms/op|
> |master (post-GROOVY-12191)|2141.1 ± 97.8 ms/op|365.9 ± 18.5 ms/op|
> |+ GROOVY-12258|2141.1 ± 97.8 ms/op|59.9 ± 1.2 ms/op|
> |+ this change|1596.1 ± 57.1 ms/op|60.5 ± 1.1 ms/op|
> Indy improves 25% over master; classic keeps the GROOVY-12258 level. On this
> worst-case category-churn bench, indy remains ~26% behind pre-GROOVY-12191:
> the residual is the relink storm (scoped domains retire many per-MetaClass
> SwitchPoints where the old design retired one global one), not the walk.
> Closing that would need a further design change — e.g. a dedicated category
> SwitchPoint axis so category enter/leave stops retiring MetaClass domains —
> and is out of scope here.
> New unit coverage includes a get/detach/drain concurrency hammer asserting no
> live SwitchPoint is ever left unregistered, exactly-once claiming between
> owner detach and bulk drain, and CAS-loser cleanup. All existing
> indy/vmplugin and category runtime tests pass.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
