builtins/call/importing/_pickle: de-leak residual per-call immortal str allocations (#171) - #838
Conversation
|
Warning Review limit reached
Next review available in: 39 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 Plus Run ID: 📒 Files selected for processing (8)
WalkthroughManaged string construction and keyword handling now use GC-aware paths. Pickle compatibility tables are cached and rooted, unpickler fields gain write barriers, and JIT root walkers forward managed children of immortal objects. ChangesGC and pickle runtime changes
Estimated code review effort: 4 (Complex) | ~60 minutes 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 44e7959). 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)
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
https://github.com/youknowone/pyre/blob/37f24c5078d992447751c7628a5df9fdcb66bbe2/pyre-interpreter/src/builtins.rs#L2945-L2946
When PYRE_GC_INTERP is enabled and a custom file object runs gc.collect() while resolving its write attribute (for example in __getattribute__ or a descriptor), s_obj is only a raw Rust local and is not yet among the roots established by call_function_impl_result; the collection can therefore sweep this newly managed string before it is passed to write, causing a dangling-reference crash or corruption. Pin and reload the string across call_method's attribute lookup; the managed literal passed by emit_literal has the same problem.
https://github.com/youknowone/pyre/blob/37f24c5078d992447751c7628a5df9fdcb66bbe2/pyre-interpreter/src/module/_pickle/mod.rs#L256-L258
For the newly supported non-dict mapping case, a mapping may legitimately return a two-element list (which PyPy accepts through space.listview) or an arbitrary malformed value, but this code immediately passes it to unsafe tuple-layout accessors and then unsafe string accessors. Replacing NAME_MAPPING with such a mapping can therefore reinterpret a list or other object as W_TupleObject and crash instead of accepting it or raising a Python exception; unpack the result through the generic sequence/type-checking APIs, and likewise validate the IMPORT_MAPPING result.
AGENTS.md reference: AGENTS.md:L194-L196
ℹ️ 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".
e3c8b20 to
243e68c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-interpreter/src/call.rs (1)
1855-1861: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRe-root pending kwargs keyword objects during dict setup.
At
bind_kwargs_to_signature,extra_kwargsholdsPyObjectRefvalues before they are pushed into a new kwargs dict. That vector is not a GC root, sow_str_from_wtf8_managed(key.clone())can be relocated while later remaining-keyword processing,w_tuple_new(extra_pos), orw_dict_new_kwargs()runs; the keys become stale beforew_dict_storeinstalls them. Keep the buffer asWtf8Bufuntil each key is needed, or pin every pending key in theextra_kwargspath, and ensurew_dict_storehas a pin between receiving the key and installing it because promotion can rebuild storage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/call.rs` around lines 1855 - 1861, Update bind_kwargs_to_signature so pending extra keyword names remain rooted while extra_kwargs is buffered and processed. Keep each name as Wtf8Buf until insertion, or pin the created keyword object before any allocation or promotion during remaining-keyword handling, w_tuple_new, and w_dict_new_kwargs. Ensure the w_dict_store path pins the key between receiving it and installing it, since storage promotion may relocate it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-interpreter/src/module/_pickle/mod.rs`:
- Around line 333-346: Root the lookup key before finditem may execute Python,
and do not reuse an unrooted mapping reference afterward. Update the surrounding
lookup flow to pin key for the duration of crate::baseobjspace::finditem, then
re-read w_name_mapping or w_reverse_name_mapping from state after the call as
needed; preserve the existing reverse-selection and result handling.
- Around line 346-366: Update the mapping lookup logic around the relevant
pickle compatibility-mapping function to validate every mapping value and tuple
element with the existing string/type predicates before calling unsafe tuple or
Unicode accessors. Preserve support for replaced or non-dict mappings, but skip
or handle entries whose value is not a two-element tuple of valid strings, and
mirror the guarded `find_class` behavior in `unpickler.rs` so malformed mappings
produce a Python-level error rather than type confusion or a panic.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/call.rs`:
- Around line 1855-1861: Update bind_kwargs_to_signature so pending extra
keyword names remain rooted while extra_kwargs is buffered and processed. Keep
each name as Wtf8Buf until insertion, or pin the created keyword object before
any allocation or promotion during remaining-keyword handling, w_tuple_new, and
w_dict_new_kwargs. Ensure the w_dict_store path pins the key between receiving
it and installing it, since storage promotion may relocate it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a44e6ca5-ca44-4843-a2d9-ef9faec72eb9
📒 Files selected for processing (8)
pyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/_pickle/mod.rspyre/pyre-interpreter/src/module/_pickle/pickler.rspyre/pyre-interpreter/src/module/_pickle/unpickler.rspyre/pyre-jit/src/eval.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 243e68c2ea
ℹ️ 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".
| let s_obj = pyre_object::w_str_from_wtf8_managed(unsafe { crate::py_str_wtf8(source)? }); | ||
| let r = crate::baseobjspace::call_method(fp, "write", &[s_obj]); |
There was a problem hiding this comment.
write
With PYRE_GC_INTERP enabled and a custom file whose __getattribute__ triggers collection, call_method resolves write before call_function pins its arguments, so this newly collectable string exists only in the untraced Rust local/slice during that lookup and can be swept before being passed to write; the same issue affects the managed default separator and terminator at line 2963. Pin and reload the string across method resolution, as translated RPython would.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
| let ptr = PICKLE_STATE.load(Ordering::Acquire); | ||
| if !ptr.is_null() { | ||
| return Some(unsafe { &*ptr }); |
There was a problem hiding this comment.
When a compatibility-map probe invokes user code—for example, a custom mapping installed before the lazy state initialization or a key comparison that triggers GC—the global root walker mutates these mapping fields through raw pointers while compat_map still holds this &'static PickleState. That violates the shared reference's immutability guarantee and also permits the compiler to reuse a pre-GC mapping pointer after the walker forwarded it, causing undefined behavior or a dangling lookup; use UnsafeCell/atomic slots or raw point-of-use loads rather than exposing a shared reference across collecting calls.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/77bfcfa15fe130f77e3e90a14a2bd5255f55eada/pyre-interpreter/src/module/_pickle/mod.rs#L340-L343
With PYRE_GC_INTERP enabled, a custom compatibility mapping or colliding mapping key can execute Python during finditem and explicitly trigger collection. The newly managed specialized tuple is held only in the Rust local key while that lookup resolves and invokes __getitem__/__eq__, so it can be swept before the lookup or error path finishes. Independently of the cached-map slot mutability issue, pin and reload this tuple across finditem.
ℹ️ 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".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyre/pyre-interpreter/src/builtins.rs (1)
2872-2996: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftUnrooted
file/positionalargs across newly-introduced managed allocations inprint().Before this PR,
print()'s string construction was immortal, so holdingfile,positional,sep,endas plain Rust locals across the emit loop was safe — nothing could be collected out from under them. This diff switches tow_str_from_wtf8_managed/w_str_new_managed, which can trigger a real GC collection (nursery/major) on allocation, andcall_method(fp, "write", ...)can additionally re-enter Python and allocate. The comments at Lines 2946-2949 and 2970-2972 acknowledge this hazard and pin only the freshly-built string — butfp(the destination file/stream) and any not-yet-processed element ofpositionalare never pinned anywhere inbuiltin_print. Once args are popped off the caller frame's value stack (the normal calling convention for a native builtin, seecall.rs's slow-pathVec<PyObjectRef>packing), these are ordinary unrooted Rust locals: a collection triggered while rendering/writing the first argument can relocate or reclaimfpor a laterpositionalelement, since nothing else keeps them reachable. The next loop iteration then reads a stale/freed pointer.Pin
file(ifSome) and every element ofpositional/sep/endup front — e.g. push one root scope before the loop, pin each, and re-read from the shadow-stack slots insideemit/emit_literalinstead of closing over the original locals.🛡️ Sketch of the needed rooting
+ let _roots = pyre_object::gc_roots::push_roots(); + let base = pyre_object::gc_roots::shadow_stack_len(); + for &p in positional { + pyre_object::gc_roots::pin_root(p); + } + if let Some(s) = sep { pyre_object::gc_roots::pin_root(s); } + if let Some(e) = end { pyre_object::gc_roots::pin_root(e); } + if let Some(f) = file { pyre_object::gc_roots::pin_root(f); } + // re-derive `positional[i]`, `sep`, `end`, `file` from `base + ...` + // via shadow_stack_get(...) inside emit/emit_literal instead of the + // closed-over locals.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/builtins.rs` around lines 2872 - 2996, Update builtin_print to root all managed objects that survive across allocations or Python re-entry: file, every positional argument, and the optional sep and end values. Establish one root scope before the emit loop, pin these values up front, and have emit, emit_literal, and loop processing use the corresponding shadow-stack slots rather than the original unrooted locals.pyre/pyre-jit/src/eval.rs (1)
10694-10702: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPossible missing write barrier on
bh_setarrayitem_gc_r.
bh_setfield_gc_r(Line 10666-10678) andbh_setinteriorfield_gc_r(Line 10734-10748) both gained an explicittry_gc_owns_object+try_gc_write_barriercall in this diff, citingllmodel.py's implicitwrite_ref_at_membarrier on every ref store.bh_setarrayitem_gc_ris the third ref-typed (_r) store in the same blackhole allocator, storing into a GC-managed array, but received no equivalent barrier here. Ifbh_setarrayitem_ref_from_descrdoesn't already barrier internally, a compiled/blackholed array-item ref store into an old-gen array can leave a young reference untracked, which an incremental mark can sweep out from under a live array — the same hazard the sibling fixes address. As per coding guidelines, the generated JIT must preserve interpreter/GC semantics rather than leaving an untranslated gap between store paths.#!/bin/bash # Check whether bh_setarrayitem_ref_from_descr already applies a write barrier. rg -n "fn bh_setarrayitem_ref_from_descr" -A 30 pyre/pyre-jit/src/eval.rs rg -n "fn bh_setarrayitem_int_from_descr|fn bh_setarrayitem_float_from_descr" -A 20 pyre/pyre-jit/src/eval.rs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-jit/src/eval.rs` around lines 10694 - 10702, Update bh_setarrayitem_gc_r to perform the same try_gc_owns_object and try_gc_write_barrier handling as bh_setfield_gc_r and bh_setinteriorfield_gc_r before storing the reference, unless bh_setarrayitem_ref_from_descr already guarantees that barrier internally. Preserve the existing array-item store behavior while ensuring GC-managed reference writes retain the required write barrier semantics.
♻️ Duplicate comments (1)
pyre/pyre-interpreter/src/module/_pickle/mod.rs (1)
337-368: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
key/w_name_mapstill unrooted across afinditemthat may run Python.This is the same concern raised on a previous revision of this exact function (unaddressed — only the sibling type-confusion comment on this code was marked fixed). The comment at Lines 337-339 claims the mapping fields are read "immediately before its allocation-free probe," but
finditem(baseobjspace.rs) falls through togetitem→__getitem__for any non-shortcut mapping — precisely the "replaced or non-dict*_MAPPING" case this code explicitly says it supports (Line 345-347). In that path the call can allocate/collect, and both the freshly builtkeytuple (Line 340-343) and thew_name_maplocal (Line 348-352) are plain Rust locals with nopin_root, unlikestate's own fields (whichwalk_pickle_state_gcdoes forward in place). A collection during that nested call leaveskey/w_name_mapstale.Pin
keyand the mapping local across thefinditemcall (and re-readstate's field afterward if reused), the same fix suggested previously.#!/bin/bash # Confirm whether baseobjspace::getitem/finditem's non-shortcut fallback # roots its `index` argument internally before invoking __getitem__. rg -n "pub fn getitem" -A 40 pyre/pyre-interpreter/src/baseobjspace.rs rg -n "pub fn finditem" -A 15 pyre/pyre-interpreter/src/baseobjspace.rs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/module/_pickle/mod.rs` around lines 337 - 368, Root the freshly created key tuple and the selected mapping local before calling crate::baseobjspace::finditem, because its fallback may execute Python and collect. Update the lookup flow around w_name_map and finditem to keep both objects valid across that call, and re-read the corresponding state mapping field afterward if the value is used again.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pyre/pyre-interpreter/src/builtins.rs`:
- Around line 2872-2996: Update builtin_print to root all managed objects that
survive across allocations or Python re-entry: file, every positional argument,
and the optional sep and end values. Establish one root scope before the emit
loop, pin these values up front, and have emit, emit_literal, and loop
processing use the corresponding shadow-stack slots rather than the original
unrooted locals.
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 10694-10702: Update bh_setarrayitem_gc_r to perform the same
try_gc_owns_object and try_gc_write_barrier handling as bh_setfield_gc_r and
bh_setinteriorfield_gc_r before storing the reference, unless
bh_setarrayitem_ref_from_descr already guarantees that barrier internally.
Preserve the existing array-item store behavior while ensuring GC-managed
reference writes retain the required write barrier semantics.
---
Duplicate comments:
In `@pyre/pyre-interpreter/src/module/_pickle/mod.rs`:
- Around line 337-368: Root the freshly created key tuple and the selected
mapping local before calling crate::baseobjspace::finditem, because its fallback
may execute Python and collect. Update the lookup flow around w_name_map and
finditem to keep both objects valid across that call, and re-read the
corresponding state mapping field afterward if the value is used again.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1e24cfd9-c2a7-4f01-988b-a98698f5ca12
📒 Files selected for processing (8)
pyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/_pickle/mod.rspyre/pyre-interpreter/src/module/_pickle/pickler.rspyre/pyre-interpreter/src/module/_pickle/unpickler.rspyre/pyre-jit/src/eval.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a6482d1d0
ℹ️ 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".
| // Rust Vec only across native-only work (the rest of this loop, | ||
| // then w_tuple_new / w_dict_new_kwargs below): no safepoint fires | ||
| // before it is installed, so the unrooted buffer stays valid. | ||
| extra_kwargs.push((pyre_object::w_str_from_wtf8_managed(key.clone()), *value)); |
There was a problem hiding this comment.
**
With PYRE_GC_INTERP enabled, an unmatched keyword key now exists only in extra_kwargs, which the collector cannot trace; allocating a subsequent managed key, the *args tuple, or w_dict_new_kwargs() can run a major collection before the keys are inserted and sweep an earlier key, leaving w_dict_store with a dangling pointer. Keep these keys on the shadow stack until the kwargs dict owns them, matching translated RPython's automatic live-rooting.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
| // the value with an unchecked tuple/str cast would instead silently fall | ||
| // back or dereference a non-str payload. | ||
| if let Some(v) = crate::baseobjspace::finditem(w_name_map, key)? { | ||
| if crate::baseobjspace::is_true(v)? { |
There was a problem hiding this comment.
When a compatibility table contains a falsy value, PyPy's if w_1 tests whether space.finditem returned a non-null wrapped object; it does not invoke Python truthiness. Calling is_true(v) therefore incorrectly ignores entries such as an empty tuple instead of letting fixedview raise, and can execute an arbitrary __bool__; if that call collects, the outer unrooted v is also stale before fixedview. Branch directly on Some(v) as the upstream structure does.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
| // lookup ever runs Python and relocates them before publication. | ||
| let _roots = pyre_object::gc_roots::push_roots(); | ||
| let base = pyre_object::gc_roots::shadow_stack_len(); | ||
| let compat = import_module("_compat_pickle").ok()?; |
There was a problem hiding this comment.
If _compat_pickle is blocked or unavailable when a protocol-0/1/2 operation first initializes this state, .ok()? silently selects identity mapping; for example, a protocol-2 pickle can emit an unmapped Python-3 global rather than failing, producing a stream that is not compatible with its intended reader. PyPy's _pickle module runs State.startup and propagates this import failure, so the lazy equivalent must return the error rather than deliberately changing the semantics.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7653a9efb7
ℹ️ 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".
| // Forward the GC-managed children of a `malloc_typed`-immortal pinned | ||
| // object, which the visit above skips. See `pyre_object_root_walker_area`. | ||
| let value = gcref.0 as pyre_object::PyObjectRef; | ||
| unsafe { pyre_interpreter::eval::walk_raw_immortal_roots(value, visitor) }; |
There was a problem hiding this comment.
With PYRE_GC_INTERP enabled, the shadow stack can contain raw GCREFs that are not PyObjectRefs—for example, capture_set_items pins a SetItemsStorage pointer and type creation pins dict_ptr. If such off-GC fallback storage is pinned while reentrant equality triggers a collection, visitor leaves it unchanged and this call reaches walk_immortal_rec, which dereferences its first word as PyObject.ob_type; that is invalid memory interpretation and can crash or corrupt the process. Preserve the root's type information or invoke this walker only for roots known to be Python objects.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pyre/pyre-interpreter/src/importing.rs (1)
1407-1425: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGood fix, but the adjacent
sys_modules_blocks(Line 1439) still does the old allocate-then-lookup — and it's on the same hot path.
check_sys_modulesnow avoids the immortalw_str_new(name)allocation by usingw_dict_getitem_str.sys_modules_blocks, called first thing bygcd_import_faston every absolute import, still builds a throwaway key viapyre_object::w_str_new(name)before looking it up (Line 1439) — the exact per-call immortal allocation this PR is otherwise removing, on a path that runs for every import.♻️ Proposed fix
fn sys_modules_blocks(name: &str) -> bool { - let key = pyre_object::w_str_new(name); let dict = sys_modules_dict(); if dict.is_null() { return false; } - match unsafe { pyre_object::w_dict_lookup(dict, key) } { + match unsafe { pyre_object::w_dict_getitem_str(dict, name) } { Some(m) => !m.is_null() && unsafe { pyre_object::is_none(m) }, None => false, } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/importing.rs` around lines 1407 - 1425, Update sys_modules_blocks to avoid constructing a temporary key with pyre_object::w_str_new(name); use the same allocation-free string-key lookup approach as check_sys_modules, while preserving the existing block lookup behavior and return semantics.pyre/pyre-interpreter/src/call.rs (1)
1790-1925: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoot or inline the managed kwargs keys before allocating the dict.
w_str_from_wtf8_managedcreates an old-gen/storable string, but when there is more than one**kwargskey,_match_kwargsleaves the first one unreachable only from the Rustextra_kwargsVec across laterw_str_from_wtf8_managed,raise_if_posonly_kwds, error formatting,w_tuple_new, andw_dict_new_kwargs()allocations. Those can each hit a GC safepoint, so the accumulated unrooted keys are not guaranteed to survive. Allocatekw_dictbefore the match loop and callw_dict_storewith the fresh managed key in the same iteration, or root the keys before any later allocation that may collect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/call.rs` around lines 1790 - 1925, Update bind_kwargs_to_signature so **kwargs keys are rooted immediately instead of accumulated as unrooted managed objects in extra_kwargs. When has_varkw is enabled, allocate kw_dict before the keyword-matching loop, create each managed key, and store it in kw_dict during the same iteration; remove the deferred extra_kwargs storage and packing path while preserving existing argument matching and error precedence.pyre/pyre-interpreter/src/module/_pickle/mod.rs (1)
549-556: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
str_from_utf8still constructs an immortal string, on the hottest path in the unpickler.
pyre_object::w_str_from_wtf8(buf)(not the_managedvariant) is used here, and this function backs everySHORT_BINUNICODE/BINUNICODE/BINUNICODE8/PERSIDopcode inunpickler.rs'sdispatch— i.e. every string value produced while unpickling. This directly contradicts the PR's stated goal of routing transient strings through managed constructors so they can be reclaimed byPYRE_GC_INTERP; unpickling any nontrivial amount of string data will leak immortal memory.🐛 Proposed fix
let buf = rustpython_wtf8::Wtf8Buf::from_bytes(data.to_vec()) .map_err(|_| unpickling_error("invalid utf-8 in pickle"))?; - Ok(pyre_object::w_str_from_wtf8(buf)) + Ok(pyre_object::w_str_from_wtf8_managed(buf))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/module/_pickle/mod.rs` around lines 549 - 556, Update str_from_utf8 to construct the decoded string with pyre_object::w_str_from_wtf8_managed instead of the immortal w_str_from_wtf8 constructor, preserving the existing UTF-8 error handling and returned PyObjectRef so transient strings created by unpickler dispatch are managed and reclaimable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pyre/pyre-interpreter/src/call.rs`:
- Around line 1790-1925: Update bind_kwargs_to_signature so **kwargs keys are
rooted immediately instead of accumulated as unrooted managed objects in
extra_kwargs. When has_varkw is enabled, allocate kw_dict before the
keyword-matching loop, create each managed key, and store it in kw_dict during
the same iteration; remove the deferred extra_kwargs storage and packing path
while preserving existing argument matching and error precedence.
In `@pyre/pyre-interpreter/src/importing.rs`:
- Around line 1407-1425: Update sys_modules_blocks to avoid constructing a
temporary key with pyre_object::w_str_new(name); use the same allocation-free
string-key lookup approach as check_sys_modules, while preserving the existing
block lookup behavior and return semantics.
In `@pyre/pyre-interpreter/src/module/_pickle/mod.rs`:
- Around line 549-556: Update str_from_utf8 to construct the decoded string with
pyre_object::w_str_from_wtf8_managed instead of the immortal w_str_from_wtf8
constructor, preserving the existing UTF-8 error handling and returned
PyObjectRef so transient strings created by unpickler dispatch are managed and
reclaimable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 19ebca84-8837-421d-8ef8-6c9d2fff9fef
📒 Files selected for processing (8)
pyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/_pickle/mod.rspyre/pyre-interpreter/src/module/_pickle/pickler.rspyre/pyre-interpreter/src/module/_pickle/unpickler.rspyre/pyre-jit/src/eval.rs
…d str alloc str()/repr() of a tagged or general object now allocate the result W_str through w_str_new_managed / w_str_from_wtf8_managed instead of the immortal w_str_new / w_str_from_wtf8, so the transient result is collectable under PYRE_GC_INTERP. print() renders each argument and the sep/end literals it hands to a file's write() through the managed constructors too, and writes the default separator / terminator straight to the native stdout stream via a new emit_literal closure instead of building a throwaway str object. Assisted-by: Claude
The three sites that pack keyword names into a dict — the builtin __pyre_kw__ marker dict, the user-function **kwargs dict, and the bind_kwargs_to_signature extra_kwargs list — build the key W_str through w_str_from_wtf8_managed instead of the immortal w_str_from_wtf8. Each key lands in a dict that is either frame-rooted (**kwargs) or consumed by the builtin before any safepoint, so the managed key stays reachable for its whole lifetime. Assisted-by: Claude
check_sys_modules keyed the sys.modules dict with an immortal W_str built from the name on every call; use the borrow-based w_dict_getitem_str, which hashes the &str directly and allocates nothing. Assisted-by: Claude
compat_map reads _compat_pickle's NAME/IMPORT mapping attributes through baseobjspace::finditem / finditem_str rather than a raw native-dict probe, so a replaced or non-dict *_MAPPING and a raising key comparison propagate like space.finditem. The function returns Result and both callers forward the error with `?`. Assisted-by: Claude
compat_map re-read the four _compat_pickle mapping tables (NAME_MAPPING, IMPORT_MAPPING, REVERSE_NAME_MAPPING, REVERSE_IMPORT_MAPPING) through a getattr on the module on every call. Import them once into a PickleState published through a static AtomicPtr, and forward the four cached dicts to the collector via walk_pickle_state_gc from walk_global_prebuilt_roots. compat_map reads the cached dicts and probes them with finditem / finditem_str; the (module, name) key is built before any cached slot is read so no unrooted local is held across an allocation. Assisted-by: Claude
…sh-stack alloc load() pins `self` before allocating the fresh w_stack/w_metastack lists and re-reads it via cur(slot) after each w_list_new. The GC write barrier is called after every store into the unpickler's w_stack / w_metastack / w_memo / w_frame / w_persistent_load fields across load, mark, pop_mark, the memo setters, set_persistent_load, the memo-proxy reset, and the FRAME opcode. Assisted-by: Claude
…tack walker pyre_object_root_walker and pyre_object_root_walker_area call walk_raw_immortal_roots on each pinned slot after visiting it, matching the pyframe value-stack/locals walker. A malloc_typed-immortal object reachable only through a pin_root shadow-stack slot (e.g. a _pickle.Unpickler across load) is skipped by the marker, so its GC-managed children were left untraced and swept. walk_raw_immortal_roots is made pub for the cross-crate call. Assisted-by: Claude
The emit / emit_literal closures on the file= path hand a fresh w_str_from_wtf8_managed / w_str_new_managed result to call_method(fp, "write", ...). The "write" attribute lookup can run a Python descriptor or __getattr__, re-entering the eval loop where the PYRE_GC_INTERP safepoint may sweep the old-gen str while it is reachable only through the native Rust local. push_roots + pin_root + shadow_stack_get root it across the call. Assisted-by: Claude
compat_map read a NAME_MAPPING hit with an unchecked w_tuple_getitem / w_str_get_value, dereferencing a non-str payload and silently falling back on a non-2-tuple value. Mirror find_class (interp_pickle.py): gate each hit on is_true, unpack the NAME_MAPPING value with fixedview(v, 2) (ValueError on wrong arity), and convert elements with text_w (TypeError on non-str), for the NAME_MAPPING and IMPORT_MAPPING branches. Document pickle_state's lazy-import plus identity-fallback divergence from State.startup. Assisted-by: Claude
Assisted-by: Claude
The (module, name) key is a managed tuple held across a generic `finditem`; a replaced or non-dict `*_MAPPING` routes the lookup through `getitem`, which can run Python and drive a collection that would sweep the unrooted key. Pin it for the lookup's duration, matching the GC-transform rooting the cached-table build already performs. Assisted-by: Claude
The **kwargs packing loops build managed keys and install them into a fresh dict in straight-line native code with no eval re-entry, so no safepoint separates a key's allocation from its store; the born-old-stable keys can be neither swept nor relocated in that window. Assisted-by: Claude
find_class's `if w_1:` is an interp-level presence test (`space.finditem` returns None when absent), not Python truthiness; drop the `is_true` gate so a present entry is selected and validated via `fixedview`/`text_w`, matching `space.listview`'s 2-unpack. Also read the cached mapping slots through raw pointers rather than a `&'static PickleState`: a generic `finditem` on a user-replaced mapping can drive a collection whose `walk_pickle_state_gc` rewrites those slots through raw pointers, which a spanning shared reference would make undefined. Assisted-by: Claude
1 participant
Follow-up to #821 (RC1 remainder). Routes the per-call immortal string allocations that the leak audit still flagged through managed/collectable constructors or borrow-based lookups, so they are reclaimed by the interp GC (
PYRE_GC_INTERP) instead of growing RSS linearly.Changes
str()/repr()of a tagged int andprint(..., file=sink)value/sep/end rendering go throughw_str_new_managed/w_str_from_wtf8_managed; the nativeprintpath writes sep/end literals straight to the stream (no alloc).w_str_from_wtf8_managed(call.rs 1851/2047/2318). The key lands in a frame-rooted**kwargsdict or a builtin marker dict consumed upfront, pure-native from create→rooted, so no dangling key.check_sys_moduleskeyssys.modulesviaw_dict_getitem_str(dict, name)(borrow) instead of allocating an immortal wrapperW_strperget_sys_module.compat_mapprobes the_compat_picklename/import maps throughfinditem/finditem_strinstead ofw_dict_lookup, propagating a raising key__eq__.RCA note
The dominant
print(i)leak was not int→str rendering (already flat via the #714/#725 managed constructors). A call-site census pinpointed two unrelated immortal-W_strsites:check_sys_modules(one immortal"sys"key perprint) and the CALL_KW keyword-name keys.Verification
check.py331/331 on dynasm, cranelift, wasm.PYRE_GC_INTERP=1:print()272→49MB,print(i,file=sink)291→70MB,print(i,file=stderr)731→66MB,f(**kw)58→63MB,str(int)47→50MB.— commented by Claude
Summary by CodeRabbit
print()behavior with customfilestreams, and more consistent default separator/line ending handling.str()andrepr()string construction for tagged values._compat_pickleremapping, better error propagation, and fastersys.moduleslookup.