jit: BUILD_TUPLE fold order and three baselines; builtins: type.__new__ arity dispatch and a buffer ljust/rjust fill - #1100
Conversation
WalkthroughThis change updates JIT tuple specialization, method receiver checks, inline snapshot publication, and benchmark statistics for blackhole adoption and store-journal rollback failures. ChangesJIT dispatch updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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
Here are some automated review suggestions for this pull request.
Reviewed commit: 83a63e10ae
ℹ️ 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".
| assert!(instance_node_setdictvalue(obj_ref, &sur, sentinel(0x55))); | ||
|
|
||
| let w_dict = pyre_object::w_dict_new_with(&MAP_DICT_STRATEGY_REF, obj_ref as *mut u8); | ||
| let w_dict = pyre_object::w_dict_new_with(&MAP_DICT_STRATEGY, obj_ref as *mut u8); |
There was a problem hiding this comment.
When the pyre-interpreter test target is compiled, this passes &MapDictStrategy to w_dict_new_with, whose signature requires &'static DictStrategyRef (pyre-object/src/dictmultiobject.rs:1316); MapDictStrategy has no conversion to that holder, so cargo test --features dynasm fails with a mismatched-type error before running the tests. Keep passing &MAP_DICT_STRATEGY_REF.
AGENTS.md reference: AGENTS.md:L236-L238
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5efa960e1
ℹ️ 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 publish_inline_frames = | ||
| (n_parents == 0 && n_callees == 1) || (n_parents > 0 && n_parents == n_callees); |
There was a problem hiding this comment.
When an inline sub-walk has one callee but no captured parent, this new arm calls the multi-frame publisher with an empty parent_frames, so the snapshot contains only the callee. On guard failure, however, eval.rs:11038-11057 and eval.rs:11450-11469 unconditionally interpret frames[0] as the outer JIT-driver frame, restore its values into the physical virtualizable, and obtain its code from the caller's vable data; thus a failure of this promote guard restores callee locals under the caller's code and omits the actual caller continuation, causing corrupt locals/namespace or a bad resume. Keep declining to the caller boundary until the caller frame can also be encoded.
AGENTS.md reference: AGENTS.md:L24-L30
Useful? React with 👍 / 👎.
![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
🤖 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-jit-trace/src/jitcode_dispatch/inline_call.rs`:
- Around line 2674-2680: Replace the pre-packing raw `method_form_receiver`
concrete snapshot with the receiver’s `OpRef` from the recorded call arguments
before vararg packing. After `w_tuple_new_array_backed` completes, resolve that
`OpRef` through `concrete_from_recorded_opref` or the equivalent `ctx.trace_ctx`
lookup, reject tagged immediates, and only then pass the forwarded object to
`pyre_object::is_type` in the FOR_ITER admission check.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs`:
- Around line 4933-4955: Run cargo check --features dynasm and cargo test
--features dynasm after the newtuple specialization paths in
try_walker_specialize_newtuple and try_walker_specialize_newtuple_object.
Execute all eight required benchmarks, record their results, and explain any
regressions before committing; do not revert parity-correct code solely due to a
measured regression.
🪄 Autofix
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: 94cb4fe1-9633-4749-a726-573ce734df69
📒 Files selected for processing (13)
pyre/bench/synth/binary_int_overflow_local_resume.cranelift.jitstatspyre/bench/synth/binary_int_overflow_local_resume.dynasm.jitstatspyre/bench/synth/binary_int_overflow_local_resume.wasm.jitstatspyre/bench/synth/exc_bridge_entry_guard_not_removed.cranelift.jitstatspyre/bench/synth/exc_bridge_entry_guard_not_removed.dynasm.jitstatspyre/bench/synth/exc_bridge_entry_guard_not_removed.wasm.jitstatspyre/bench/synth/list_append_write_barrier_gc.cranelift.jitstatspyre/bench/synth/list_append_write_barrier_gc.dynasm.jitstatspyre/bench/synth/list_append_write_barrier_gc.wasm.jitstatspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs
| // The receiver a method-form call passes as its first positional argument. | ||
| // Read before the vararg packing below, which folds that argument into the | ||
| // surplus tuple whenever the callee has no positional parameter to hold it | ||
| // and so leaves the first seeded local holding the tuple instead. The | ||
| // FOR_ITER admission gate further down asks whether the receiver is a type | ||
| // object, and a tuple would answer that question for the wrong object. | ||
| let method_form_receiver = callee_arg_concretes.first().copied(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Do not keep the receiver as a raw pointer across tuple allocation.
When vararg packing runs, pyre_object::w_tuple_new_array_backed allocates a tuple at Line [2715]. That allocation can move a nursery object. method_form_receiver stores the pre-forwarding ConcreteValue::Ref, and Line [2981] uses it after the allocation. The pyre_object::is_type call can then read a stale pointer.
Save the receiver OpRef before packing. Resolve its forwarded concrete value after packing through concrete_from_recorded_opref or the equivalent ctx.trace_ctx lookup. Reject tagged immediates before calling pyre_object::is_type; the tuple specializer documents that they have no real object header.
Use the forwarding channel for the receiver
- let method_form_receiver = callee_arg_concretes.first().copied();
+ let method_form_receiver_op = callee_args.first().copied();
...
- || method_form_receiver.is_some_and(|concrete| {
+ || method_form_receiver_op
+ .map(|opref| concrete_from_recorded_opref(ctx, opref))
+ .is_some_and(|concrete| {
matches!(
concrete,
ConcreteValue::Ref(receiver)
- if !receiver.is_null() && !unsafe { pyre_object::is_type(receiver) }
+ if !receiver.is_null()
+ && !pyre_object::tagged_int::is_tagged_int(receiver)
+ && !unsafe { pyre_object::is_type(receiver) }
)
});Also applies to: 2977-2981
🤖 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-trace/src/jitcode_dispatch/inline_call.rs` around lines 2674 -
2680, Replace the pre-packing raw `method_form_receiver` concrete snapshot with
the receiver’s `OpRef` from the recorded call arguments before vararg packing.
After `w_tuple_new_array_backed` completes, resolve that `OpRef` through
`concrete_from_recorded_opref` or the equivalent `ctx.trace_ctx` lookup, reject
tagged immediates, and only then pass the forwarded object to
`pyre_object::is_type` in the FOR_ITER admission check.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 3bd7873). 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
|
…ding `try_walker_specialize_newtuple` runs before `try_walker_specialize_newtuple_object` again, and the two dispatch comments in `residual_call` go back to describing that order. With arity 2 declined in the canonical fold the two arms cover disjoint arities, so the order no longer decides which one claims a pair. `try_walker_specialize_newtuple_object`'s header and the concrete-shadow comment go back to naming arity 2 as declined. A paragraph records what a side exit does when the decline is lifted: the trace hands a real pair, with inline `value0` / `value1` and no `wrappeditems` block, to a consumer picked for the canonical layout, and `try_walker_specialize_subscr_specialised_pair` reads a field that is not there. Assisted-by: Claude
`walker_capture_inline_nonstandard_vable_guard` published the callee's own resume coordinate only when the paused-caller chain covered the full inline depth (`n_parents > 0 && n_parents == n_callees`). A chain of one callee frame and no paused caller is covered by the frame list just as directly, so it takes the same publish; every other shape still falls through to the single-frame sentinel. `walker_capture_multi_frame_inline_snapshot` reaches `publish_outermost_parent_vable_scalars` with an empty `parent_frames` on that new shape, so the call is made only when there is a parent to publish. Assisted-by: Claude
`type_descr_new` located `(name, bases, dict)` by scanning the positional
arguments for the first `str` with two more behind it, and took the metatype as
the last preceding type. `descr__new__` (typeobject.py:886-911) keys the
one-versus-three form on `len(__args__.arguments_w)` instead, runs
`_precheck_for_new` unconditionally after that decision, and only then reads the
name. The scan reached `type_descr_new_with_metaclass` before the branch that
prechecks, so a non-type metatype was dropped rather than refused and
`type.__new__(42, 'A', (), {})` built a class.
The scan's header cited a `[self, metatype, name, bases, dict]` shape from
`super()`. No receiver is prepended: `super().__new__(mcls)` inside a metaclass
raises `M.__new__() takes exactly 3 arguments (1 given)`, which is the one-element
`pos` arm, and `super().__new__ is type.__new__` holds with `__self__` the type.
`pos[0]` is always the metatype.
`pos` being empty is a shape upstream's gateway cannot produce, since it supplies
`w_typetype` from a declared parameter; it is refused here in `tp_new_wrapper`'s
words rather than through the three-argument message.
`_calculate_metaclass` (typeobject.py:945) now receives the bases as written.
`(object,)` is substituted for an empty tuple in `W_TypeObject.__init__`
(`bases_w or [space.w_object]`), after the winner is settled; supplying it earlier
weighed an explicit metatype against `type(object)` and reported a metaclass
conflict for `type.__new__(int, 'A', (), {})`. The winner then passes
`check_user_subclass` (typeobject.py:555-567), which `allocate_instance` runs on
the way in and which is what names `int is not a subtype of type`.
`_ast`'s heap-type creation passed `[name, bases, dict]` and relied on the scan
matching at index 0; it now passes the metatype. The `_ctypes` metaclasses
already forwarded `[metatype, name, bases, dict]`.
`bool.__new__`'s over-arity refusal reported `got more`, the only such wording in
the tree; the gateway counts the class argument along with the value.
Assisted-by: Claude
`pad_fillchar` refused every non-`str` fill, and its own header recorded the gap: "The decode itself is not imported". `descr_ljust` and `descr_rjust` (unicodeobject.py:1352,1371) convert the operand with `convert_arg_to_w_unicode` (unicodeobject.py:175-184), which declines `bytes` by name and hands everything else to `decode_object` (unicodeobject.py:1727-1739); that reads the operand as a buffer and decodes it strict UTF-8, so a `bytearray` or `memoryview` becomes a fill character. `descr_center` (unicodeobject.py:1101) reads its operand with `space.utf8_w` and is unchanged. The `"decoding to str: ..."` arm now reports only an operand that exports no buffer at all, which is what `decode_object` maps the buffer error to. A buffer whose bytes are not valid UTF-8 raises from the decode. The single-character check applies to the decoded operand, so a multi-byte fill is rejected by its code-point count. Assisted-by: Claude
youknowone
changed the title
1 participant
Four commits against
main: two JIT, two builtins.1.
check.pyis red onmainfor three benches#1063 was squash-merged at the commit that restored
try_walker_specialize_newtuple_object's arity-2 decline, but the baselinere-record lived in the commit after it.
maintherefore carries the decline inits code and the pre-decline numbers in its baselines:
binary_int_overflow_local_resumeexc_bridge_entry_guard_not_removedlist_append_write_barrier_gc(
bridges_compiled/guard_failures, identical on all three backends.)The measured values are byte-for-byte what these files held before the arity-2
virtualization landed: the extra bridges were that fold's
mixed-representation side exits, and declining it again removed them. The
re-record restores those counters and keeps the
fbw_*/field_pos_*keysthe branch added.
The same commit puts
try_walker_specialize_newtupleback ahead oftry_walker_specialize_newtuple_objectinresidual_call, which is the orderevery comment in both functions describes. With arity 2 declined the two arms
cover disjoint arities, so the order no longer decides which claims a pair —
this removes a deviation, not a behaviour. A paragraph records what a side
exit does if the decline is ever lifted: the trace hands a real pair, with
inline
value0/value1and nowrappeditemsblock, to a consumer pickedfor the canonical layout, and
try_walker_specialize_subscr_specialised_pairreads a field that is not there.
2. The single-callee-frame vable promote guard
walker_capture_inline_nonstandard_vable_guardpublished the callee's ownresume coordinate only when the paused-caller chain covered the full inline
depth (
n_parents > 0 && n_parents == n_callees). A chain of one callee frameand no paused caller is covered by the frame list just as directly, so it takes
the same publish; every other shape still falls through to the single-frame
sentinel, which is the shape that cannot resume wrong.
walker_capture_multi_frame_inline_snapshotreachespublish_outermost_parent_vable_scalarswith an emptyparent_frameson thatnew shape, so the call is made only when there is a parent to publish.
3.
type.__new__dispatched on a str scan instead of the argument counttype_descr_newlocated(name, bases, dict)by scanning the positionalarguments for the first
strwith two more behind it, then took the metatypeto be the last preceding type object. Nothing on that path called
_precheck_for_new, and it returned before the arm that does, sotype.__new__(42, 'A', (), {})created a class where pypy raisesX is not a type object (int). Two of the eight measured rows were a wronganswer rather than a wrong message.
descr__new__(typeobject.py:886-911) switches on the count and nothing else:w_typetype = pos[0],arguments_w = &pos[1..], count first — touchingw_typetypeonly to word the refusal — then_precheck_for_new, then theone-argument
type(x)case, then_create_new_type. That is what thisimplements. Three things the port needed beyond the switch:
pos.is_empty(). Upstream's gateway suppliesw_typetypefrom a declaredparameter, so
descr__new__never sees that shape. pyre reads it out of thesame flat slice and refuses it here, in
tp_new_wrapper's words._astcaller.module/_ast/moduledef.rsbuilt[name, bases, dict]with no metatype and relied on the scan matching at index 0; a count-first
port silently reinterprets that as
w_typetype = name. It now passescrate::typedef::w_type()._ctypes/metaclass.rsalready forwarded the4-argument shape.
calculate_metaclassmust see the bases as written. pyre substituted(object,)for an empty tuple before the winner was computed;W_TypeObject.__init__does it after (bases_w or [space.w_object]). Thatsubstitution is what reported
metaclass conflictfortype.__new__(int, 'A', (), {})— it weighedintagainsttype(object),where upstream refuses the metatype itself. With a base written out both
weigh the same thing and already agreed, so only the empty tuple diverged.
int is not a subtype of typehas no producer indescr__new__: it comes fromW_TypeObject.check_user_subclass(typeobject.py:555-567), whichspace.allocate_instance(W_TypeObject, w_typetype)runs on the way in.The header comment justifying the scan claimed
super()may prepend a boundreceiver, giving
[self, metatype, name, bases, dict]. It does not.super().__new__(mcls)inside a metaclass raisesM.__new__() takes exactly 3 arguments (1 given)—pos == [M]— identical topypy3 character for character; prepended it would be
[type, M]and would havereturned
<class 'type'>. pyre's descriptor kind is also right:type.__dict__['__new__']is abuiltin_function_or_methodwhose__self__isthe owning type, in pyre and CPython alike. The calling convention needed no
change.
Also here:
bool(1, 2)reportedgot more, the only instance of that wordingin the tree; it now carries the gateway's count wording.
4. A buffer
ljust/rjustfill operandpad_fillchar's own comment already named the gap ("the decode itself is notimported").
convert_arg_to_w_unicode(unicodeobject.py:175-184) sends anon-
stroperand throughdecode_object→space.bufferstr_w+ strict utf-8,so
'x'.ljust(5, bytearray(b'-'))pads. The decode is strict, sobytearray(b'\xff')raisesUnicodeDecodeErrorrather than being refused bytype, and a multi-byte fill is rejected by its code-point count, not its byte
count.
centertakesspace.utf8_wand is unchanged.Coverage
extra_tests/parity_tests/builtin_new_argument_count.pyandstr_padding_fill_operand.py. Both branch onsys.implementation.name, whichlets one fixture pin both sides of a deliberate divergence — CPython 3.14
dropped the one-argument
type.__new__form and refuses a buffer fill, andthose rows are asserted under the
cpythonarm. Both fixtures pass underpypy3unmodified, which is what proved the target wording before any Rust waswritten.
A 44-row three-way matrix over these shapes now has zero rows matching
neither reference.
Rebased onto d936eb4
One conflict. #1097 deleted
widened_method_foriter_admissibleoutright, alongwith
method_form_callee_body_supportedandInlineBodyFacts::method_form_supported— the type-receiver attribute readfolds now, so the proof that gate demanded is no longer what admits the callee.
The commit that fixed how the gate read its operand had nothing left to fix;
main's deletion is taken and the
method_form_receiverbinding it introduced isdropped with it. That commit now carries only the
resume_snapshot.rschangedescribed in §2 and is worded for it.