[
https://issues.apache.org/jira/browse/GROOVY-12259?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18105097#comment-18105097
]
ASF GitHub Bot commented on GROOVY-12259:
-----------------------------------------
daniellansun commented on PR #2786:
URL: https://github.com/apache/groovy/pull/2786#issuecomment-5307523007
## Summary
The three commits form a coherent progression:
1. Skip the all-`ClassInfo` walk when this process has never allocated a
SwitchPoint.
2. Replace that walk with a process-wide registry of live SwitchPoints, so
category enter/leave is O(live domains).
3. Stop that registry from retaining entries after a domain is discarded
without an explicit detach.
I think the destination is the right one. The part I would like to discuss
is commit 3’s orphan-reaper protocol: it is correct as far as I can follow, but
it turns `SwitchPointInvalidator` from a small lazy cell into a process-wide GC
protocol, and it pumps that protocol from `getSwitchPoint()`, which is the MOP
link path. I wonder whether the same leak- and staleness-freedom can be
obtained by giving `ClassInfo` a class-level domain that outlives the
`MetaClass` object — analogous to the pending domain it already keeps — and
leaving the live set as the straightforward map introduced in commit 2.
None of this is a claim that the present code is wrong. It is a suggestion
that a different ownership boundary might delete a whole layer of mechanism.
---
## What I think is working well
- **Register-before-publish.** An empty registry observation really does
mean there was nothing a bulk path needed to retire. That is a clean
replacement for the monotonic flag in `5013c5f`.
- **Keying the registry by `SwitchPoint`, not by invalidator.** A single-use
SwitchPoint cannot ABA-clobber a successor the way an invalidator-keyed
`remove` could. The comment that explains this is worth keeping, wherever the
map ends up.
- **Two-argument `remove` for a single claimant.** Drain and reaper cannot
both retire the same orphan. That handshake is easy to follow.
- **Replacing `isAnySwitchPointAllocated` with `hasLiveSwitchPoints()`.**
The flag never re-armed, so a process that linked once would pay the walk
forever. The registry check is the better model.
- **The tests around concurrent get / detach / drain**
(`registryInvariant_underConcurrentGetDetachAndDrain`, the CAS-loss leak bound)
give a reader something concrete to trust.
---
## 1. Would it be possible to keep domain lifetime on `ClassInfo`?
`SwitchPointInvalidator` previously did one job: allocate a SwitchPoint
lazily, detach it, invalidate it. After commit 3 it also owns:
- a process-wide `ConcurrentHashMap<SwitchPoint, OwnerRef>`
- a `ReferenceQueue` and a `WeakReference` subclass
- opportunistic reaping
- bulk drain, including a second “owner already dead” mode
- a test hook (`clearOwnerRefForTesting`) that deliberately breaks the
weak-ref invariant
The comments have to restate the same JVM fact several times —
`guardWithTest` keeps only the SwitchPoint’s internal invoker, not the
SwitchPoint object — which I took as a sign that the lifetime model is no
longer obvious from the types.
The two production owners are not actually the same kind of thing:
| Owner | What is collected | What I believe we need |
|---|---|---|
| `ClassInfo.pendingIndySwitchPoint` | a discarded script `Class` /
`ClassInfo` | the call sites die with the class; the registry entry should not
linger |
| `IndyInvalidation.DOMAINS` | a soft/weak `MetaClass` collected while the
`Class` is still live | exact-class invalidation must still be able to find
that domain |
Commit 3 treats both as “owner died, so invalidate the orphan from a global
queue.” That is a conservative and understandable choice. I am not sure it is
the smallest one, and I am not sure it fully closes the second case.
Exact-class retirement still goes through `ClassInfo`:
```java
// IndyInvalidation
public static void collectLiveForClass(final Class<?> type, final
List<SwitchPoint> out) {
ClassInfo.getClassInfo(type).collectLiveIndySwitchPoints(out);
}
// ClassInfo
public void collectLiveIndySwitchPoints(final List<SwitchPoint> out) {
IndyInvalidation.collectLiveForMetaClass(getMetaClassForClass(), out);
SwitchPoint pending = pendingIndySwitchPoint.detachLive();
if (pending != null) {
out.add(pending);
}
}
```
After a soft `MetaClass` is collected, `getMetaClassForClass()` returns
`null` and `hasClassLevelMetaClass()` is false, so the next install is treated
as a first install. `invalidateClass`, `incVersion`, and a stock registry
replace then have nothing to detach. GroovyObject sites typically pin the
`MetaClass` via `SAME_MC.bindTo(mc)`; an optimised POJO handle often does not.
Those POJO sites can remain on a still-valid SwitchPoint that exact-class
invalidation can no longer see.
The reaper runs only from `getSwitchPoint()` and `drainLive()`. A warmed-up
process whose sites are already linked, whose `MetaClass` has been softly
collected, and which is not using categories, calls neither. That is close to
the “latent staleness window” described in the commit 3 message. The protocol
will close the window once something else links or a category `use` runs; it
will not close it from `invalidateClass` itself.
A direction you have already used, and that I would be grateful if you would
consider extending:
- `ClassInfo` already keeps `pendingIndySwitchPoint` for the pre-MC
generation.
- A class-level invalidator that outlives the `MetaClass` *object* the same
way would let `invalidateClass` find the domain after a soft collection.
- “Weak MC gone, installing a new one” could be treated as replace, not as
first install.
- The live set could remain the commit 2 map (`SwitchPoint →
SwitchPointInvalidator`), without weak values.
- Discarded-script cleanup could live on `ClassInfo` collection.
`finalizeReference()` already exists, though as far as I can see it is not
invoked from `ClassValue` / `globalClassSet`. Wiring that, or accepting that a
dead class’s pending entry sits until the next bulk drain, would avoid a third
lifetime world on the cell.
If that is workable, `OwnerRef`, `ORPHANS`, `reapOrphans()`, the drain-time
orphan branch, and `clearOwnerRefForTesting` could all go away, and exact-class
invalidation would see the domain again. If it is *not* workable — for example
if a class-level handle would pin something you have been careful not to pin —
I would very much like to understand that constraint. I may simply have the
reachability wrong.
---
## 2. `getSwitchPoint()` as the reaper pump
```java
public SwitchPoint getSwitchPoint() {
// Allocation is the operation churn-heavy processes keep performing,
// so it doubles as the reaper pump; a no-op while the queue is empty.
reapOrphans();
for (;;) {
SwitchPoint sp = current.get();
if (sp != null) {
return sp;
```
The comment describes allocation as the pump. The poll happens *before* the
live-`current` hit, so every MOP link (`classSwitchPointFor` →
`getSwitchPoint()`) pays a `ReferenceQueue.poll()`. That call is inexpensive
when the queue is empty, but it is still a process-wide synchronised check.
After a category `use` block — the path this series is making cheaper — every
site re-links, and every re-link takes that lock. If the queue is not empty,
the link also waits on one-at-a-time `invalidateAll`s.
I realise an empty poll is cheap, and that you called this out in the commit
message (“Stable processes pay … an empty queue poll on the link path”). My
hesitation is only whether that cost belongs on the link path at all, given
that the series is otherwise moving work *off* the category / re-link path.
`drainLive` then does some of the same work twice, in two different styles:
```java
static void drainLive(final List<SwitchPoint> out) {
reapOrphans();
LIVE.forEach((sp, ref) -> {
SwitchPointInvalidator inv = ref.get();
if (inv == null) {
if (LIVE.remove(sp, ref)) {
out.add(sp);
}
} else if (inv.detachIfCurrent(sp)) {
out.add(sp);
}
});
}
```
`reapOrphans()` invalidates immediately. The `forEach` already claims
`ref.get() == null` into `out` so that
`IndyInvalidation.retireAllLoadedDomains` can use a single `invalidateAll`.
Reaping first turns orphans that could have been batched into single
invalidations, then hides them from the batch. After script churn plus `use`,
the category path therefore does the slower thing first.
It also splits the contract of `drainLive`: some SwitchPoints are
invalidated inside the method, others are only detached into `out`.
`retireAllLoadedDomains` still reads as “drain, then `invalidateBatch`,” which
is no longer the whole story.
If the orphan protocol remains, two modest adjustments would already make
the control flow easier to follow:
1. Do not call `reapOrphans()` from `drainLive()`. The `forEach` is enough,
and it keeps one `invalidateAll`.
2. Do not poll on the `getSwitchPoint()` hit path. If a pump is required
when there is no drain, `java.lang.ref.Cleaner` registered at domain creation
would at least keep it off the link path.
---
## 3. Small leftovers from the walk that the registry replaced
These are minor and only worth a pass if you are already editing the
comments.
- `retireAllLoadedDomains` no longer walks loaded domains. A name such as
`retireLiveDomains` would match what the method now does.
- `ClassInfo.detachLiveIndySwitchPoint` still says to prefer
`collectLiveIndySwitchPoints` for bulk paths. Bulk paths now go through
`SwitchPointInvalidator.drainLive`.
- `hasLiveSwitchPoints()` before `drainLive()` is a reasonable GROOVY-12258
fast path; an empty `drainLive` is already cheap. I do not feel strongly about
keeping or folding it.
The public `isAnySwitchPointAllocated()` from `5013c5f` is already gone in
`f549626`, which seems right for an unreleased 6.0 surface.
---
## 4. Tests, if you pursue the lifetime change
The new unit tests (`drain_claimsOrphanWhoseOwnerWasCollected`,
`reaper_invalidatesOrphanedSwitchPointAfterOwnerGc`) pin down the map / queue
handshake clearly. If the design stays as it is, they are the right tests.
If you are willing to consider moving lifetime back onto `ClassInfo`, the
behaviours I would most want a later reader to see are:
- After a soft `MetaClass` collection, `invalidateClass` / `incVersion`
still retires a still-installed guard.
- A discarded script class does not leave `hasLiveSwitchPoints()` true
indefinitely.
- A `use` block after script churn batch-invalidates; it does not
single-invalidate via `reapOrphans()`.
A smaller observation on the current tests, offered only as a consistency
note:
- `reaper_invalidatesOrphanedSwitchPointAfterOwnerGc` waits on `System.gc()`
for up to two seconds. That is reasonable if the design is “GC, then a later
`get`.” A `ClassInfo`-owned domain would not need that test.
- `drain_claimsOrphanWhoseOwnerWasCollected` ends with `inv.detachLive()`
because the owner is still alive and `current` is stale. `getSwitchPoint()`
will return that already-invalidated SwitchPoint (`current != null` is
sufficient). Production avoids that only because a true orphan’s owner is dead.
The test hook therefore encodes “the owner is dead,” not “`current` is a live
registered SwitchPoint.” That may be acceptable; I mention it only because it
is a slightly fragile invariant for a later editor.
---
## A possible shape, if you find it useful
I would personally be happy to see:
1. Commit 2’s registry, keyed by `SwitchPoint`, register-before-publish,
deregister on detach — that *is* the GROOVY-12259 fix.
2. A class-level domain handle on `ClassInfo` (or a `ManagedReference`
finalize on the weak `MetaClass`) so exact-class invalidation survives
`MetaClass` collection.
3. Explicit `ClassInfo` cleanup for discarded scripts, so the live set
cannot grow without bound.
4. No `OwnerRef` / `ReferenceQueue` / link-path pump.
File size is not a concern (`SwitchPointInvalidator` is 345 lines). The
question is only cohesion: three retirement modes (owner detach, drain detach,
reaper invalidate) versus one live set plus domain ownership on `ClassInfo`.
I may have under-estimated a pinning or AOT constraint that makes the
class-level handle unattractive. If so, I would be glad to be corrected. Thank
you for the careful write-up in the commit 3 message — it made the intended
invariant much easier to review.
> 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)
