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.
--
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]
