interp: generator GC finalization + parity review follow-ups (#549)#559
Conversation
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
✨ Finishing Touches🧪 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 |
A generator suspended inside a try/finally, except, or with block did not run its cleanup when garbage-collected while still suspended; only an explicit close() ran it. Register a GC finalizer for a generator whose code object has a non-empty exception table, and dispatch a generator-typed finalizer object to generator_finalize (generator.py _finalize_): when the suspended frame is still live and its current instruction is covered by an exception-table handler, raise GeneratorExit into the frame so the pending finally/except/ with cleanup runs. - FrameBox::into_generator registers the finalizer via register_generator_finalizer when the code has any exception handler. - UserDelAction::_call_finalizer branches on is_generator to generator_finalize instead of the __del__ lookup. - generator_finalize no-ops when the frame is finished (frame_ptr null) or not started (last_instr < 0). PyPy gates registration on the compile-time CO_YIELD_INSIDE_TRY flag; that flag is unavailable here because the compiler is an external dependency, so registration gates on the runtime exception table, a necessary condition for any reachable finally/except/with. Assisted-by: Claude
… generator throw - _abc_init: default a base's __abstractmethods__ only on AttributeError (propagate other errors), iterate any iterable, and probe an inherited method with getattr so a descriptor-provided implementation counts. - Type creation: raise "duplicate base class X" for a repeated direct base, and run C3 validation inside type.__new__ (default-metaclass path) instead of before dispatch, so a custom metaclass owns its bases. - list.__init__ appends incrementally via list.extend, retaining a completed prefix when a later next() fails; list.extend's storage fast path applies only to exact list/tuple so a subclass __iter__ override is honored. - _functools.cmp_to_key builds the stdlib K wrapper: __slots__ = ['obj'], the comparator captured in the closure (no public cmp, no instance dict), comparisons compare other.obj without a same-type gate. - Class-mode super(C, C).attr calls a descriptor __get__ with None as the object argument. - generator.throw normalizes into the generator only on success; a non-exception or normalization failure is raised to the caller. yield-from delegation forwards the original (type, value, traceback) throw arguments. Assisted-by: Claude
![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: fa9ff8f698
ℹ️ 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 let Some([w_type, w_val, w_tb]) = throw_args { | ||
| // PEP 380 / generator.py delegation forwards the original three | ||
| // throw arguments, rather than a synthesized exception instance. | ||
| return crate::call::call_function_impl_result(throw, &[w_type, w_val, w_tb]); |
There was a problem hiding this comment.
When outer.throw(...) is delegated through yield from to a non-generator iterator, this always calls the delegate's throw with three positional arguments, even if the caller supplied only one or two. CPython forwards the original argument list, so an iterator with def throw(self, typ): ... works for outer.throw(ValueError), but this path raises an argument-count TypeError or changes observable *args behavior. Track the original throw-argument count or store a slice/Vec rather than padding with None before forwarding.
Useful? React with 👍 / 👎.
| // app_abc.py:69 — normal getattr lets descriptors and | ||
| // metaclass attributes provide a concrete implementation. | ||
| let value = match crate::baseobjspace::getattr_str(cls, unsafe { | ||
| pyre_object::w_str_get_value(name) |
There was a problem hiding this comment.
Because this change now accepts any iterable __abstractmethods__, a base can provide something like [1]; the reference getattr(cls, name, None) raises TypeError for a non-string name, but this unsafe w_str_get_value(name) casts the arbitrary object as a W_UnicodeObject, which can read invalid memory or panic instead of surfacing a Python exception. Check is_str(name) and raise the appropriate error (or route through an object-level getattr) before extracting the string.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 5e4f249). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
…rect
The wasm backend routed every `CallMayForce{I,R,N}` residual call through
the host `jit_call` trampoline (guest->host->guest: reflect the callee
signature, marshal args through linear memory, call, write the result
back), while the non-forceable residual families
(`Call{,Pure,Loopinvariant}{I,R}`) already took a direct in-module
`call_indirect` into the callee's table slot. Since every `int`/`bool`
binary operation compiles to a `CallMayForceR` (descr `[Ref,Ref,Int] ->
Ref`, a uniform-i64 ABI), an int-heavy loop paid one ~158 ns host
crossing per operation: `arith_int_bool` issued 10.7M of them, which was
its entire wasm-vs-native gap.
`CallMayForce` was excluded only for parity conservatism: on RPython the
`jf_descr` force-descr is stored immediately before a CALL_MAY_FORCE so
`guard_not_forced` can observe a forced virtualizable. The wasm backend
has no force tokens — the virtualizable is always materialized and
`GuardNotForced` is lowered as an always-pass no-op — and the host
trampoline performs no `jf_descr` store either, so a direct call is
sound and matches RPython's own direct-CALL lowering. Extend
`residual_call_i64_arity` (`CallMayForceI|CallMayForceR`) and
`residual_call_void_word_arity` (`CallMayForceN`); the arity type-section
census and trampoline census follow through `direct_helper_i64_arity`.
Float / release-GIL / cond / assembler calls and non-reflectable descrs
stay on the trampoline.
arith_int_bool: jit_calls 10,705,693 -> 16,418; wasm 1.85s -> 0.55s
(11.1x -> 3.1x vs dynasm). No effect on traces without uniform-i64
CallMayForce ops (byte-identical modules). check.py --backend wasm
185/185 (list_insert_pop_index BASEFAIL is a local-oracle artifact).
Assisted-by: Claude
Assisted-by: Claude
Assisted-by: Claude
|
Addressed both Codex review findings and rebased onto current
Also on this branch: a wasm-JIT perf commit (
— commented by Claude |
1 participant
Follow-up to #549 (merged), rebased on current
main. Two correctness commits;python3 pyre/check.pypasses every synthetic bench on both backends and all touched behaviors match the CPython oracle. (Locally one unrelated bench,list_insert_pop_indexfrom #552, BASEFAILs only because this machine's CPython oracle is 3.9 — itslist.insert(float)message predates 3.11; pyre already emits the 3.11+/3.14 message.)Generator finalization on GC
A generator suspended inside a
try/finally,except, orwithblock did not run its cleanup when garbage-collected while still suspended — only an explicitclose()did. It now registers a GC finalizer (gated on a non-empty code exception table, since PyPy's compile-timeCO_YIELD_INSIDE_TRYflag is unavailable — the compiler is an external dependency) and, on collection, raisesGeneratorExitinto the frame when its current instruction is covered by a handler. Verified across try/finally,with__exit__, nested try, andyield fromdelegate cleanup. New snippetgenerator_gc_finalize.py.Parity review follow-ups (from #549's Codex review)
_abc_initdefaults a base's__abstractmethods__only onAttributeError(propagates other errors), iterates any iterable, and probes inherited methods withgetattrso a descriptor implementation counts.duplicate base class 'X'; runs C3 validation insidetype.__new__(default-metaclass path) instead of before dispatch, so a custom metaclass owns its bases.list.__init__appends incrementally vialist.extend(retains a completed prefix on a mid-iteration failure);list.extend's storage fast path applies only to exactlist/tupleso a subclass__iter__override is honored.Kwrapper —__slots__ = ['obj'], comparator captured in the closure (no publiccmp, no instance__dict__), comparisons without a same-type gate.super(C, C).attrcalls a descriptor__get__withNoneas the object argument.yield fromdelegation forwards the original(type, value, traceback)triplet.🤖 Generated with Claude Code