majit: keep a boxing cluster's w_class as its own store - #1197
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)
WalkthroughModule names now use traced Python objects instead of externally allocated WTF-8 buffers. Constructors, accessors, GC traversal, module initialization, and reachability checks preserve name objects, including lone surrogates. ChangesModule name representation and collection
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🔵 Low · up to The PR changes runtime module-name handling and garbage-collection layout. It is mergeable with owner awareness, but the required JIT validation and benchmark results should be provided as follow-up. Sequence Diagram(s)sequenceDiagram
participant Module
participant GCTraversal
participant Reachability
participant SysModules
Module->>Module: store name in w_name
Module->>Module: seed __name__ with the same object
GCTraversal->>Module: trace w_name
Reachability->>Module: read w_name
Reachability->>SysModules: compare module identity
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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/33a155e97c9c3efc0322a1e4fedab7aefdd7a52e/pyre-object/src/module.rs#L230-L231
When ModuleType.__init__ is applied to an existing bootstrap module such as sys, this stores a potentially GC-managed string in an off-heap malloc_typed module. The write barrier does not make that field a root: walk_module_dicts_gc only visits module.w_dict, while W_MODULE_GC_PTR_OFFSETS applies only to managed module objects. A program can therefore reinitialize sys with a dynamically created name, delete sys.__name__, and collect the string; release_frees_nothing later dereferences the dangling w_name during shutdown. The immortal-module root walker must visit w_name as well.
ℹ️ 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".
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 9df3324). 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
|
e28b31a to
47941f9
Compare
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/e28b31a8f96b0175af53e60e286ca976ba68ea1f/pyre-interpreter/src/typedef.rs#L21507
When building a non-sandbox Windows target, this narrower cfg removes the mmap_buffer_view branch even though that helper and the mmap implementation are explicitly available under cfg(any(unix, windows)). Consequently buffer_as_bytes_like returns None for Windows mmap objects, causing consumers such as bytes concatenation, numeric parsing, codecs, and marshal to reject a valid buffer; retain the previous any(unix, windows) platform condition.
ℹ️ 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.
💡 Codex Review
https://github.com/youknowone/pyre/blob/47941f970e9712980b3ec0d95a99c0ee6611446e/pyre-interpreter/src/typedef.rs#L21581
On Windows, this narrower cfg compiles out the mmap buffer path even though mmap_buffer_view remains implemented under cfg(any(unix, windows)). Consequently, buffer-consuming operations routed through buffer_as_bytes_like—notably bytes(m), bytearray(m), concatenation, and joins—no longer recognize a Windows mmap; constructor fallback iteration yields one-byte bytes objects rather than integers and raises TypeError instead of copying the mapping. Preserve the previous any(unix, windows) gate.
AGENTS.md reference: AGENTS.md:L231-L233
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f012ab2469
ℹ️ 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 !module.w_name.is_null() { | ||
| visitor(&mut module.w_name); |
There was a problem hiding this comment.
When the immortal module created by ExecutionContext::get_builtin is reinitialized with a collectable string and its dictionary's __name__ entry is subsequently deleted or overwritten, this walk never reaches the new w_name: it scans only MODULE_DICT_ROOTS, while that module is held in builtin_dict_cache and is never passed to set_sys_module. The EC root walker merely forwards the unmanaged module pointer, so a major collection can sweep the name and release_frees_nothing can later dereference stale memory if the module is released from a user global during shutdown. Trace w_name for every immortal module owner, not only modules registered in sys.modules.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
youknowone
changed the title
|
Rebased onto current The What remains is the single majit commit, and the description above has been — commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df126ff8dd
ℹ️ 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".
| // payloads. Each is a plain `FieldWrite` the assembler lowers to its | ||
| // own `setfield_gc`, which is the shape `jtransform.py:1044` leaves | ||
| // every field the allocation itself does not carry. | ||
| for (k, payload) in site.w_class.into_iter().chain(site.payloads).enumerate() { |
There was a problem hiding this comment.
When w_class differs from the vtable-derived class (notably w_long_from_raw and bytes-subclass allocation), this chain now emits a FieldWrite carrying the original PyObject.w_class descriptor immediately after NewWithVtable. The runtime descriptor bridge resolves that access to the canonical w_class_descr, whose parent descriptor is deliberately None, but OptVirtualize::optimize_setfield_gc unconditionally calls get_parent_descr().expect(...) for every non-typeptr field on a virtual allocation. Because NewWithVtable is virtualized before this adjacent store is processed, compiling either newly fused path panics instead of producing a trace; handle w_class as a special virtual header field (and preserve its explicit value) before enabling this fusion.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
`fuse_boxing_alloc` dropped both header stores and let the `NewWithVtable`'s
vtable stamp `ob_type` and `w_class`, so it could only fire where the cluster's
`w_class` was `get_instantiate(&T)` for the very `&T` its `ob_type` named.
Anything else declined and left `malloc_typed` residual, which the rtyper
rejects fail-closed; the whole graph then census-Skips to the legacy walker.
`jtransform.py:1040-1045 rewrite_op_malloc` emits `new_with_vtable` carrying
only the sizedescr and leaves every other field to its own `setfield`. Do the
same with `w_class`: lower the cluster and re-emit the store as a `FieldWrite`
on the new object whenever the vtable does not stand for it. Predecessors that
disagree — one folding into the vtable and another not, or two naming different
classes — still decline, as they already did for a disagreeing `ob_type`
(`malloc.py:176-186`).
`prune_dead_boxing_remnants` needs no change — its liveness gate already keeps
a producer the flow reaches, which the re-emitted store now makes the `w_class`
feed.
The reject population over the pyre sources goes from 36 lines across 18 graphs
to 4 across 4. `w_long_from_raw` pairs `&LONG_TYPE` with
`get_instantiate(&INT_TYPE)` so that `type(1 << 100) is int`, and is the root
cited by 14 of those 18; the remaining 4 are `w_generator_or_coroutine_new` and
its three callers, which decline for the other reason — three `ob_type`
constants merge into one malloc, so no single vtable stands for the cluster —
and are exactly the 4 that survive.
Clearing the reject is not the same as lifting the graph. Of the 18: 9 now
annotate, 4 keep that reject, and 5 fail on a different error it was standing
in front of (`compute_at_fixpoint`, on `unary_negative_value` /
`unary_invert_value` under both spellings and on `PositionCookie::to_object`).
Across the whole census phaseA goes 1497 -> 1487. Diffing the failing-graph
sets rather than the totals decomposes that into 14 fixed and 4 newly failing —
a different 14 from the one above: the 9 that now annotate plus 5 that had been
failing only through a cascade from them (`__pyre_wrap_random`,
`w_dict_proxy_new`, `w_float_new`, `w_range_compute_item`, `w_range_new`).
The 4 that regress are `PyFrame::getorcreate_debug_data` and its three callers:
UnionError in mergeinputargs: longobject::W_LongObject ∪ pyframe::FrameDebugData
In <FunctionGraph of write>
`core::ptr::write` is lifted as one graph shared by every instantiation
(`Lowering::new` takes the declaration's `fd.generics`; the instantiation lives
at the call site), and the raw write survives fusion by design
(`sink_fused_boxing_aggregates_at_raw_writes`). Until now only `FrameDebugData`
reached it; `w_long_from_raw` lifting means `W_LongObject` reaches it too, and
slot 0 unions two instances with no common base. Any change that lets
`w_long_from_raw` lift arms this, so it is not something a narrower variant of
this patch avoids. The upstream shape is per-instantiation specialization —
`Specializer::Argtype` (`specialize:argtype(N)`) is already implemented — but
nothing in production sets `annspecialcase` on an LLBC-lifted function, so
wiring it is a front-end change of its own. Those 4 fall back to the legacy
walker, the same fail-closed path the reject already had them on the edge of.
Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9df332441c
ℹ️ 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 Some(header) = resolve_header_plan(graph, agg) else { | ||
| continue; |
There was a problem hiding this comment.
When a registered host-static address is 0, resolve_header_plan now returns Some(HeaderPlan { vtable: 0, ... }) because resolved? checks only for absence, and this site no longer preserves the previous explicit vtable == 0 rejection. The resulting NewWithVtable reaches codewriter/assembler.rs, whose new_with_vtable arm deliberately panics on a zero vtable, turning a cluster that previously remained a fail-closed residual malloc_typed into a translation failure. Keep the cluster unfused when header.vtable == 0.
Useful? React with 👍 / 👎.
1 participant
fuse_boxing_allocdropped both header stores and let theNewWithVtable'svtable stamp
ob_typeandw_class, so it could only fire where the cluster'sw_classwasget_instantiate(&T)for the very&Titsob_typenamed.Anything else declined and left
malloc_typedresidual, which the rtyperrejects fail-closed; the whole graph then census-Skips to the legacy walker.
Change
jtransform.py:1040-1045 rewrite_op_mallocemitsnew_with_vtablecarryingonly the sizedescr and leaves every other field to its own
setfield. Do thesame with
w_class: lower the cluster and re-emit the store as aFieldWriteon the new object whenever the vtable does not stand for it.
Predecessors that disagree — one folding into the vtable and another not, or two
naming different classes — still decline, as they already did for a disagreeing
ob_type(malloc.py:176-186).prune_dead_boxing_remnantsneeds no change: its liveness gate already keeps aproducer the flow reaches, which the re-emitted store now makes the
w_classfeed.
Measurement
Both arms run on identical LLBC — only
majit-translate/src/model.rsdiffers(
PYRE_LLBC_STRICT=0demotes the repo-wide #1164 digest, which a translator-onlyswap necessarily trips).
mainsurvived fuse_boxing_alloc unfusedPREPASS phaseA failw_long_from_rawpairs&LONG_TYPEwithget_instantiate(&INT_TYPE)so thattype(1 << 100) is int, and is the root cited by 14 of those 18. The other 4are
w_generator_or_coroutine_newand its three callers, which decline for theother reason — three
ob_typeconstants merge into one malloc, so no singlevtable stands for the cluster — and are exactly the 4 that survive.
Clearing the reject is not the same as lifting the graph. Of the 18: 9 now
annotate, 4 keep that reject, and 5 fail on a different error it was standing
in front of (
compute_at_fixpoint, onunary_negative_value/unary_invert_valueunder both spellings and onPositionCookie::to_object).The −10 is not uniform: 14 fixed, 4 newly failing
Diffing the failing-graph sets rather than the totals decomposes the −10 into
14 fixed and 4 regressed. (That 14 is a different set from the 14 above: the 9
that now annotate, plus 5 that had been failing only through a cascade from them
—
__pyre_wrap_random,w_dict_proxy_new,w_float_new,w_range_compute_item,w_range_new.)The 4 that regress are
PyFrame::getorcreate_debug_dataand its three callers:core::ptr::writeis lifted as one graph shared by every instantiation(
Lowering::newtakes the declaration'sfd.generics; the instantiation livesat the call site), and the raw write survives fusion by design —
sink_fused_boxing_aggregates_at_raw_writes' test asserts "the raw write mustsurvive the rewrite". Until now only
FrameDebugDatareached that graph;w_long_from_rawlifting meansW_LongObjectreaches it too, and slot 0 unionstwo instances with no common base.
This is a latent landmine rather than a defect in this rewrite: any change
that lets
w_long_from_rawlift arms it, so there is no narrower variant ofthis patch that avoids it. The upstream shape is per-instantiation
specialization —
Specializer::Argtype(specialize:argtype(N)) is alreadyimplemented — but nothing in production ever sets
annspecialcaseon anLLBC-lifted function, so wiring it is a front-end change of its own and is left
as follow-up. The 4 graphs fall back to the legacy walker, the same fail-closed
path the reject already had them on the edge of.
Widening the union instead would be the wrong repair: the analogous
List ∪ Inst(classdef-less)arm lifted 5 graphs and was reverted because phaseBsilently accepted the resulting erasure.
Verification
cargo test -p majit-translate --release --lib— 3190 passed, 0 failed.python3 pyre/check.py --backend dynasm— 425/425;--backend cranelift— 425/425.type(1 << 100) is int, hot-loop-x/~xarithmetic, andclass B(int)subclass identity all correct.🤖 Generated with Claude Code