diff --git a/dev/modules/README.md b/dev/modules/README.md index 6982e0e8f..45a3fb6be 100644 --- a/dev/modules/README.md +++ b/dev/modules/README.md @@ -20,6 +20,7 @@ This directory contains design documents and guides related to porting CPAN modu | [math_bigint_bignum.md](math_bigint_bignum.md) | Math::BigInt / BigFloat / BigRat / bignum support (in progress) | | [storable_binary_format.md](storable_binary_format.md) | Storable native Perl binary format — read + write paths landed; jperl ↔ system-perl files interoperate in both directions | | [unicode_collate.md](unicode_collate.md) | Unicode::Collate — plan: file-backed DUCET + Java XS surface (default); optional ICU path tradeoffs | +| [ppi.md](ppi.md) | **PPI** — CPAN test status, RC1–RC4, refcount/`DESTROY` follow-ups (`t/04_element.t` `%_PARENT`) | ## Module Status Overview diff --git a/dev/modules/ppi.md b/dev/modules/ppi.md index 4240b4f49..a6f03bdcb 100644 --- a/dev/modules/ppi.md +++ b/dev/modules/ppi.md @@ -258,3 +258,77 @@ file before closing the phase. - Is the `return $r` bug a separate issue (return copy discipline) or a symptom of the same container-store miscount? Needs a separate minimal repro without containers. + +--- + +## Future work — `t/04_element.t` and `%PPI::Singletons::_PARENT` (2026-05) + +Upstream PPI keeps a weak parent table: `weaken($_PARENT{refaddr $child} = $parent)`. +`PPI::Element::DESTROY` deletes `$_PARENT{refaddr $_[0]}`. +`PPI/tests/t/04_element.t` asserts that after parsing a large document and +destroying the root `PPI::Document`, `scalar keys %_PARENT` returns to the +baseline count (tests **202** explicit `DESTROY`, **206** implicit scope exit). + +### Symptom on PerlOnJava (as of 2026-05) + +Those two subtests still fail: thousands of `_PARENT` keys remain after +document teardown, i.e. many `PPI::Element` objects never run `DESTROY` (or +equivalent cleanup). Other failures in `./jcpan -t PPI` (e.g. `t/07_token.t`, +`t/08_regression.t`) may share refcount/DESTROY plumbing; treat them after +`04_element` is green. + +### Fixes landed incrementally (see linked PR) + +1. **`InheritanceResolver`**: do **not** negative-cache a failed `DESTROY` + method lookup. During `require`/compile order, resolution can run before + a base class installs `DESTROY`; caching `null` would skip Perl destructors + for the rest of the process. +2. **`DestroyDispatch.doCallDestroy`**: set `destroyFired` only **after** it + is known that a Perl `DESTROY` (or the no-DESTROY cleanup path) will run. + Setting it before the null-check could skip `PPI::Element::DESTROY` when + resolution transiently failed. +3. **`RuntimeScalar.incrementRefCountForContainerStore`**: activate nested + referents at `refCount == -1` before incrementing (and refuse `MIN_VALUE` / + `WEAKLY_TRACKED`), so hash/array slot stores and `createReferenceWithTrackedElements` + paths can count anon containers consistently. + +These improve destructor reliability but **do not** yet pass tests 202/206 alone. + +### Next investigation steps (ordered) + +1. **Reproduce at scale** + `./jperl -Ilib t/04_element.t` (or only the failing scope) with `PERL5LIB` + pointing at bundled `IO::File` etc. Use `timeout` on full `./jcpan -t PPI` + (see `AGENTS.md`). + +2. **Correlate stuck keys with JVM state** + For one leaked element after `$doc->DESTROY`, check + `Internals::jperl_refstate`-style introspection (if available): `refCount`, + `refCountOwned`, whether the object is still reachable from a live + `RuntimeArray` slot (`{children}`) or from deferred mortal queues. + +3. **`PPI::Node::DESTROY` pattern** + BFS: `unshift @queue, @{delete $_->{children}}` then `%$_ = ()`. Ensure + `RuntimeHash.delete` and bulk clear both drop nested refcount for **non-`refCountOwned`** + slot scalars where Perl still drops SV refcount (prior art: a narrowly scoped + `deferDecrementRecursive` or hash-delete helper broke unrelated tests when + too broad). + +4. **Hash / anon literal construction** + JVM path uses `RuntimeHash.createHashRef` → `createReferenceWithTrackedElements`. + A mid-construction `MortalList.flush` could in theory destroy nested values + before container-store pins them; `suppressFlush` around **only** the hash-ref + construction sequence was attempted but caused regressions elsewhere — any + retry must be narrower than wrapping all `createHashRef` callers. + +5. **After `04_element` passes** + Re-run full `./jcpan -t PPI`, refresh the failure table at the top of this + doc, then continue Phase 2/RC2 refcount parity work from "Next Steps" above + for lexer/`Structure::Condition` issues. + +### References + +- `lib/PPI/Node.pm` (`DESTROY`), `lib/PPI/Element.pm` (`DESTROY`), + `lib/PPI/Singletons.pm` (`%_PARENT`) +- Runtime: `DestroyDispatch.java`, `MortalList.java`, `RuntimeHash.delete`, + `RuntimeScalar.incrementRefCountForContainerStore` diff --git a/src/main/java/org/perlonjava/runtime/mro/InheritanceResolver.java b/src/main/java/org/perlonjava/runtime/mro/InheritanceResolver.java index 4eb4f9fa6..e9d2bb47c 100644 --- a/src/main/java/org/perlonjava/runtime/mro/InheritanceResolver.java +++ b/src/main/java/org/perlonjava/runtime/mro/InheritanceResolver.java @@ -184,6 +184,35 @@ public static void invalidateCache() { DestroyDispatch.invalidateCache(); } + /** + * Drop method-resolution cache entries that depend on a given package sub's + * leaf name (stash key {@code My::Pkg::foo} → {@code foo}), including + * {@code \0noautoload} variants. Also clears {@link DestroyDispatch} bookkeeping + * when the leaf is {@code DESTROY}. + */ + public static void invalidateMethodLookupCachesForStashSubKey(String stashFqn) { + if (stashFqn == null || stashFqn.isEmpty()) { + return; + } + int chop = stashFqn.lastIndexOf("::"); + String leaf = chop < 0 ? stashFqn : stashFqn.substring(chop + 2); + if (leaf.isEmpty()) { + return; + } + String suffix = "::" + leaf; + String suffixNoAutoload = suffix + "\0noautoload"; + methodCache.entrySet().removeIf(e -> { + String k = e.getKey(); + return k.endsWith(suffixNoAutoload) + || k.endsWith(suffix) + || k.equals(leaf) + || k.equals(leaf + "\0noautoload"); + }); + if ("DESTROY".equals(leaf)) { + DestroyDispatch.invalidateCache(); + } + } + /** * Retrieves a cached OverloadContext for the given blessing ID. * @@ -429,7 +458,9 @@ public static RuntimeScalar findMethodInHierarchy(String methodName, String perl } } - // Cache the fact that method was not found (using null) + // Cache "method not found" as null. Late-installed or redefined package subs + // invalidate matching entries via GlobalVariable.globalCodeRefs and in-place CV + // updates on stash-backed RuntimeScalars (see globalCodeRefFqn). methodCache.put(cacheKey, null); return null; } diff --git a/src/main/java/org/perlonjava/runtime/runtimetypes/DestroyDispatch.java b/src/main/java/org/perlonjava/runtime/runtimetypes/DestroyDispatch.java index e6296ca12..e9ed3fa97 100644 --- a/src/main/java/org/perlonjava/runtime/runtimetypes/DestroyDispatch.java +++ b/src/main/java/org/perlonjava/runtime/runtimetypes/DestroyDispatch.java @@ -250,13 +250,6 @@ public static void callDestroy(RuntimeBase referent) { * Perform the actual DESTROY method call. */ private static void doCallDestroy(RuntimeBase referent, String className) { - // Mark as destroyed before running DESTROY — one-shot guard. - // Prevents re-entrant DESTROY if cascading cleanup brings this - // object's refCount to 0 again within the same call stack. - // Also prevents infinite DESTROY loops for rescued objects - // (destroyFired stays true after rescue — see note in rescue path). - referent.destroyFired = true; - // Use cached method if available RuntimeScalar destroyMethod = destroyMethodCache.get(referent.blessId); if (destroyMethod == null) { @@ -283,6 +276,13 @@ private static void doCallDestroy(RuntimeBase referent, String className) { return; } + // Mark as destroyed only once we will run Perl DESTROY. Setting destroyFired + // before the null check above incorrectly skips PPI::Element::DESTROY (and + // any other Perl destructor) when method resolution transiently returned null + // or fell through without a CODE ref — leaving PPI::_PARENT keys orphaned. + // Prevents re-entrant Perl DESTROY for the same object; see callDestroy. + referent.destroyFired = true; + // If findMethodInHierarchy returned an AUTOLOAD sub (because no explicit DESTROY // exists), we need to set $AUTOLOAD before calling it. The method resolver sets // autoloadVariableName on the RuntimeCode when it falls through to the AUTOLOAD pass. diff --git a/src/main/java/org/perlonjava/runtime/runtimetypes/GlobalVariable.java b/src/main/java/org/perlonjava/runtime/runtimetypes/GlobalVariable.java index 6f0c6ec34..bec936770 100644 --- a/src/main/java/org/perlonjava/runtime/runtimetypes/GlobalVariable.java +++ b/src/main/java/org/perlonjava/runtime/runtimetypes/GlobalVariable.java @@ -32,7 +32,7 @@ public class GlobalVariable { // Maps fully-qualified names (package::subname) to indicate they should be called // as user-defined subroutines instead of built-in operators public static final Map isSubs = new HashMap<>(); - public static final Map globalCodeRefs = new PackageRootMap<>(); + public static final Map globalCodeRefs = new GlobalCodeRefMap(); static final Map globalIORefs = new HashMap<>(); static final Map globalFormatRefs = new HashMap<>(); @@ -113,6 +113,75 @@ public void clear() { } } + /** + * Tracks FQN on stash-backed code scalars and invalidates method-resolution cache + * lines that depend on the sub's leaf name when the map entry is meaningfully + * created or replaced (see {@link InheritanceResolver#invalidateMethodLookupCachesForStashSubKey}). + */ + private static final class GlobalCodeRefMap extends HashMap { + private static void maybeInvalidateMethodCacheForCodeRefPut(String key, RuntimeScalar previous, RuntimeScalar value) { + if (key == null || value == null) { + return; + } + boolean reseat = previous != null && previous != value; + boolean firstDefinedInstall = previous == null && RuntimeCode.isCodeDefined(value); + if (reseat || firstDefinedInstall) { + InheritanceResolver.invalidateMethodLookupCachesForStashSubKey(key); + } + } + + @Override + public RuntimeScalar put(String key, RuntimeScalar value) { + markPackageGlobalRoot(value); + RuntimeScalar old = super.put(key, value); + invalidatePackageRootSnapshot(); + if (old != null && old != value) { + old.globalCodeRefFqn = null; + } + if (value != null) { + value.globalCodeRefFqn = key; + } + maybeInvalidateMethodCacheForCodeRefPut(key, old, value); + return old; + } + + @Override + public RuntimeScalar putIfAbsent(String key, RuntimeScalar value) { + RuntimeScalar existing = get(key); + if (existing != null) { + markPackageGlobalRoot(existing); + return existing; + } + return put(key, value); + } + + @Override + public RuntimeScalar remove(Object key) { + RuntimeScalar prev = super.remove(key); + if (prev != null) { + invalidatePackageRootSnapshot(); + prev.globalCodeRefFqn = null; + } + if (key instanceof String s) { + InheritanceResolver.invalidateMethodLookupCachesForStashSubKey(s); + } + return prev; + } + + @Override + public void clear() { + if (!isEmpty()) { + for (RuntimeScalar s : values()) { + if (s != null) { + s.globalCodeRefFqn = null; + } + } + invalidatePackageRootSnapshot(); + } + super.clear(); + } + } + static T markPackageGlobalRoot(T root) { if (root == null) return null; root.isPackageGlobalRoot = true; diff --git a/src/main/java/org/perlonjava/runtime/runtimetypes/RuntimeScalar.java b/src/main/java/org/perlonjava/runtime/runtimetypes/RuntimeScalar.java index 770bc5186..f407b2087 100644 --- a/src/main/java/org/perlonjava/runtime/runtimetypes/RuntimeScalar.java +++ b/src/main/java/org/perlonjava/runtime/runtimetypes/RuntimeScalar.java @@ -78,6 +78,14 @@ private static boolean mightBeInteger(String s) { */ public boolean ioOwner; + /** + * When this scalar is installed in {@link GlobalVariable#globalCodeRefs}, the map key + * (fully-qualified name such as {@code My::Pkg::foo}). Used to invalidate + * method-resolution cache lines for that sub's leaf name after in-place CV updates + * via {@link #set(RuntimeScalar)}. + */ + public String globalCodeRefFqn; + /** * Number of closures that have captured this RuntimeScalar variable. * When {@code captureCount > 0}, {@link #scopeExitCleanup} skips the @@ -942,19 +950,38 @@ public void vivifyLvalue() { * constructor, and their refCount was already incremented at creation time. */ public static void incrementRefCountForContainerStore(RuntimeScalar scalar) { - if (scalar != null && !scalar.refCountOwned - && (scalar.type & REFERENCE_BIT) != 0 && scalar.value instanceof RuntimeBase base - && base.refCount >= 0) { - base.refCount++; - base.hadCountedReference = true; - base.recordOwner(scalar, "incrementRefCountForContainerStore"); - base.recordActiveOwner(scalar); - scalar.refCountOwned = true; - // Phase B1 (refcount_alignment_52leaks_plan.md): track the - // container element so ReachabilityWalker can see it via - // ScalarRefRegistry. - ScalarRefRegistry.registerRef(scalar); + if (scalar == null || scalar.refCountOwned + || (scalar.type & REFERENCE_BIT) == 0 + || !(scalar.value instanceof RuntimeBase base)) { + return; + } + // Destroyed / weaken sentinel states must not be reactivated here. + if (base.refCount == Integer.MIN_VALUE + || base.refCount == WeakRefRegistry.WEAKLY_TRACKED) { + return; } + // Anonymous nested containers start at refCount=-1 (untracked). + // bless {}'s retroactive pass and normal container stores must still count + // the hash slot as a strong ref, otherwise delete $h{k} / hash clear never + // drops the child (PPI::Node->{children}; cpan PPI t/04_element.t). + // Restrict promotion to real containers: doing this for arbitrary -1 + // referents (e.g. refs to readonly scalars) breaks Moo weak_ref accessors + // (cpan Moo t/accessor-weaken.t). + if (base.refCount < 0) { + if (!(base instanceof RuntimeHash) && !(base instanceof RuntimeArray)) { + return; + } + base.refCount = 0; + } + base.refCount++; + base.hadCountedReference = true; + base.recordOwner(scalar, "incrementRefCountForContainerStore"); + base.recordActiveOwner(scalar); + scalar.refCountOwned = true; + // Phase B1 (refcount_alignment_52leaks_plan.md): track the + // container element so ReachabilityWalker can see it via + // ScalarRefRegistry. + ScalarRefRegistry.registerRef(scalar); } private static boolean scalarReferenceContentsNeedRetain(RuntimeScalar value) { @@ -1232,6 +1259,9 @@ private RuntimeScalar setLargeRefCounted(RuntimeScalar value) { ScalarRefRegistry.registerRef(this); } + int preAssignType = this.type; + Object preAssignValue = this.value; + // Do the assignment this.type = value.type; this.value = value.value; @@ -1385,6 +1415,17 @@ private RuntimeScalar setLargeRefCounted(RuntimeScalar value) { DestroyDispatch.clearRescuedWeakRefsTo(oldBase); } + if (isPackageGlobalRoot && globalCodeRefFqn != null) { + boolean invalidate = preAssignType != this.type || preAssignValue != this.value; + if (!invalidate && this.type == CODE && this.value instanceof RuntimeCode nc + && preAssignType == CODE && preAssignValue instanceof RuntimeCode oc) { + invalidate = oc.defined() != nc.defined(); + } + if (invalidate) { + InheritanceResolver.invalidateMethodLookupCachesForStashSubKey(globalCodeRefFqn); + } + } + return this; }