threading: defer post-fork weakref cleanup to a safe dispatch point - #1154
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughThe PR adds explicit deferred old-generation collection requests, tracks nested evaluation depth, handles requests at interpreter and JIT safepoints, and schedules collection after child-side fork. The threading DynASM baseline changes from ChangesDeferred collection flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ForkChild
participant request_oldgen_collection
participant EvalLoop
participant safepoint
participant OldGenerationGC
ForkChild->>request_oldgen_collection: queue deferred collection
request_oldgen_collection->>EvalLoop: arm GC eval-breaker
EvalLoop->>safepoint: process EB_GC or EB_GC_INTERP
safepoint->>OldGenerationGC: collect old generation at eligible depth
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 44f0783). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f073f4074e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !requested && !enabled() { | ||
| return; | ||
| } | ||
| if !at_outermost_activation() { |
There was a problem hiding this comment.
When PYRE_GC_INTERP is off (the native default), EvalActivationGuard::enter() does not increment EVAL_NESTING, so this new gate always reports root completeness. If os.fork() is called from a re-entrant Python callback such as a sorted() key function, the child services the explicit request at the callback's next opcode even though the outer native opcode handler still holds unregistered PyObjectRefs on its Rust stack; the non-moving major can free those old-generation objects and leave dangling references. Explicit requests need an activation counter that remains accurate even when interpreter allocation routing is disabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5af881f: EvalActivationGuard now tracks per-thread nesting regardless of PYRE_GC_INTERP, while armed continues to represent only the allocation-routing feature gate.
| if requested { | ||
| majit_gc::collector::request_deferred_major_collection(); | ||
| } |
There was a problem hiding this comment.
With PYRE_GC_INTERP=1, a child fork performed at callback depth 3 or greater reaches this branch on every subsequent opcode: the safepoint consumes EB_GC and immediately sets it again. Until the callback unwinds, interpreter dispatch repeatedly enters this residual and compiled backedges repeatedly deopt because EB_GC is in JIT_BREAKER_MASK; a callback that continues a long loop after fork() can therefore suffer unbounded slowdown and bridge churn. Preserve the deferred request outside the active breaker bit and re-arm it only after returning to an eligible activation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5af881f: refused semantic requests remain in a process-global pending bit outside EB_GC; note_eval_activation_exit re-arms EB_GC once when depth becomes eligible. The re-entrant sorted(key=...) fork fixture now completes immediately with JIT on and off.
| if is_fresh { | ||
| w_type_add_fresh_subclass(w_base, w_self); | ||
| } else { | ||
| w_type_add_subclass(w_base, w_self); |
There was a problem hiding this comment.
When a custom metaclass mro() re-entrantly assigns cls.__bases__ during construction, type_set_bases calls w_type_ready_existing and registers the nascent class on its new base before the outer construction resumes. This branch then assumes the class is fresh and blindly adds it again, so the base's __subclasses__() contains the same class twice and invalidation traverses it twice. Initial ready() must retain upstream add_subclass's identity scan rather than introducing this fresh-only shortcut, consistent with the repository's strict structural-parity requirement.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by removing the heap-type optimization commit from the PR entirely. The diff now retains PyPy w_type_add_subclass identity scan and dead-slot replacement ordering unchanged.
f073f40 to
5af881f
Compare
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5af881f2c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // here would run while this native builtin still | ||
| // owns unregistered Rust-stack temporaries, and a | ||
| // moving full collection would be unsafe. | ||
| pyre_object::gc_interp::request_oldgen_collection(); |
There was a problem hiding this comment.
When automatic GC is disabled and the child inherits an unreachable cycle, this unconditional request still sweeps the entire old generation at the next opcode, clearing weakrefs and potentially scheduling finalizers that neither CPython nor PyPy runs merely because fork() returned. For example, after gc.disable(); a.self = a; r = weakref.ref(a); del a, CPython preserves r() in the child across os.fork(), while this path collects a; fixing _MainThread's acyclic refcount-timing difference with a process-wide tracing pass therefore changes unrelated observable lifetime semantics and conflicts with the required PyPy structural parity.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
1 participant
Summary
The earlier heap-type optimization was removed after parity review; subclass registration remains on PyPy add_subclass semantics.
Verification
Summary by CodeRabbit