Skip to content

test: flush GraalPy's inline caches before checking a class is collectable (#1963) - #2013

Merged
JarryShaw merged 1 commit into
mainfrom
test/1963-graalpy-cache-collectable
Oct 21, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
test/1963-graalpy-cache-collectable

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • test — tests only

Description of your pull request and other information

On GraalPy, test_cache_is_per_class_and_collectable failed in the sharded runs of #2012, but not in an unsharded run. The cause is not #2010's late finaliser.

GraalPy's attribute and call nodes cache the classes they see, and hold them strongly until the node goes megamorphic. A plain class stays pinned by a cls.mro() call alone, with no pcapkit code involved. Waiting does not free it: a class stayed alive through 30 s of repeated gc.collect(). It was only released once more classes went through the same nodes. A shard reaches the test with those caches still holding classes, whereas an unsharded run has already filled them.

On GraalPy only, if the class survives the first gc.collect(), the test now pushes INLINE_CACHE_FLUSH throwaway classes of the same shape through info_final and _nested_info_types, then collects again. The gate is sys.implementation.name. On CPython the flush count is 0, so the check runs exactly as before.

Reproduced on GraalPy 25.4.4: the test method run repeatedly in one process failed in 7 of 20 runs before, and passed 20 of 20 after (200 of 200 under --vm.Xmx400m). The test still catches a real leak. With a class-pinning WeakKeyDictionary re-added, it fails on both GraalPy and CPython.

Part of #1963.

@JarryShaw JarryShaw added test Pull requests that add or correct tests (test: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 21, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 21, 2026
@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 21, 2026
)

test_cache_is_per_class_and_collectable failed on GraalPy 25.4.4 in
sharded runs. The class is not freed late: graalpython's
LookupAttributeInMRONode.SlowPath.lookupConstantMRO caches the receiver
class strongly, up to AttributeAccessInlineCacheMaxDepth classes, until
lookupCachedLen replaces it (graal-25.4.4). So a class measured while a
node in the nested-type walk still has room stays reachable however long
gc.collect is given. Unsharded runs passed because earlier tests had
already overflowed those caches.

On GraalPy only, when the first collection leaves the class alive, send
throwaway classes of the same shape through the walk and collect again.
The smallest flush that frees the class every time equals the option's
value, measured at the default and a raised one; the constant is
headroom. A reinstated module-level WeakKeyDictionary pin still fails on
both interpreters. CPython runs the check unchanged.
@JarryShaw
JarryShaw force-pushed the test/1963-graalpy-cache-collectable branch from 7e5d9ba to 00179a5 Compare October 21, 2026 04:38
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at 00179a57e. A Sonnet review of 7e5d9ba27 found the code and gate sound:

  • the flush branch is dead on CPython, so the check is unchanged there;
  • the flush never touches pcapkit-held state, so a real leak still fails;
  • the tier guard passes.

It asked for a source for the inline-cache claim, and 00179a57e adds one. I checked the source myself: at graal-25.4.4, LookupAttributeInMRONode.SlowPath.lookupConstantMRO caches @Cached("klass") Object cachedKlass strongly, whereas the cached-MRO specialisation uses weak = true. It holds up to AttributeAccessInlineCacheMaxDepth classes, 5 by default.

Measured:

  • The option sets the count. Setting it to 2 left 2 classes stuck, and setting it to 10 left 10.
  • Smallest flush that always frees. It equals the option's value, so 64 is headroom.
  • Flush disabled vs enabled. With the flush disabled, 7 of 20 runs failed. With it enabled, 0 of 20 failed.

The test file passes on CPython (14) and on GraalPy (14).

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage: 91.55% (unit tier, Python 3.15, 00179a57e, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 19597 881 2618 867 92.11%
pcapkit/corekit 2542 71 810 37 96.54%
pcapkit/dumpkit 276 3 100 3 98.40%
pcapkit/foundation 2930 23 1022 25 98.79%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 17501 222 4612 187 98.13%
pcapkit/toolkit 872 36 298 10 95.21%
pcapkit/utilities 523 2 158 4 99.12%
pcapkit/vendor 4431 1995 1018 167 51.73%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit f96a6b1 into main Oct 21, 2026
57 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 21, 2026
@JarryShaw
JarryShaw deleted the test/1963-graalpy-cache-collectable branch October 21, 2026 14:08
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 21, 2026
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Pull requests that add or correct tests (test: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant