jit: restore the raise-bearing method-form inline, and inline a user binop/compare dunder that reads self.attr - #954
Merged
Merged
Conversation
…oad_attr
The previous commit computed `widened_method_form` for the FOR_ITER admission
but keyed the raise decline on `allow_method_load_attr`. Five entries pass that
flag; four of them -- the `type.__call__` `__init__` fold, the exception
`__str__`/`__repr__` override, and the property getter and setter -- passed it
before the body-reads-`self.attr` widening, so the decline also withdrew inlines
that were already happening.
A method-form body with no attribute read is one the narrow surface admits, so
`method_form_callee_body_supported` returns true for it and
`widened_method_form` is false. Measured on
class B:
def bump(self, n):
if n < 0:
raise ValueError(n)
return n + 1
for i in range(400000): t += b.bump(i)
min of 7 interleaved runs, startup subtracted:
dynasm origin/main 0.011s, before 0.504s, after 0.022s
cranelift origin/main 0.021s, before 0.418s, after 0.018s
`PYRE_FBW_INLINE_DIAG` prints the same 8 `[inline-body]` lines as origin/main
again, and `loops_compiled` returns from 2 to 1.
`synth/inline_subwalk_mutating_residual` keeps its gain: against an origin/main
binary, min of 7 interleaved, startup subtracted, 0.225s -> 0.075s (dynasm) and
0.363s -> 0.089s (cranelift).
check.py: dynasm 357/357, cranelift 357/357.
Assisted-by: Claude
`try_walker_inline_user_binop` and `try_walker_inline_user_compareop` were the
last two entries passing `allow_method_load_attr = false`, so
`method_form_callee_body_supported` declined any dunder body carrying a
`LoadAttr` residual -- `def __lt__(self, o): return self.x < o.x`, the ordinary
shape. Pass `true` there.
With that, all seven `try_walker_inline_resolved_user_call` call sites pass the
same value, so the parameter and the branch it selected are gone;
`widened_method_form` drops its now-constant conjunct and keeps naming the
bodies the two declines are scoped to.
There is no upstream counterpart to the check being removed.
`codewriter/policy.py:35` `look_inside_function` returns True by default,
`_reject_function` (:38-46) rejects only elidable functions and
`rpython.rtyper.module.*` helpers, and `look_inside_graph` (:48-64) rejects only
loop-bearing graphs, `_jit_look_inside_` overrides and unsupported variable
types -- all on RPython graphs, never on an app-level body. App-level dunders
dispatch uniformly through `descroperation.py:706`.
Measured, 4,000,000 iterations of a `while` loop, min of 5 interleaved runs,
startup subtracted, against a binary built from origin/main:
def __lt__(self, o): return self.x < o.x
dynasm 3.469s -> 0.056s
cranelift 3.961s -> 0.093s
def __add__(self, o): return self.x + o.x
dynasm 3.609s -> 0.047s
cranelift 4.053s -> 0.066s
The `for` form is unchanged: the FOR_ITER `DeferredCall` admission denies
`arg_class_guard.is_some()`, which is exactly these two entries, and that denial
is what keeps the `BINARY_OP` rewind from resuming one operand short.
Seven semantics probes in `while` form, 200000 iterations each, match CPython on
both backends and on an origin/main binary: NotImplemented falling through to
the reflected operand; an AttributeError raised inside the body escaping as
AttributeError with the TypeError counter at 0; a `__getattr__` fallback; a
property getter's read count landing on exactly 200000 per operand; a
proper-subclass rhs keeping reflected priority; a ValueError raised once well
after the trace is hot; and the receiver's attribute mutating mid-loop. An
eighth pins a store committed before the failing rhs map guard at exactly one
execution per iteration.
check.py: dynasm 358/358, cranelift 358/358.
Assisted-by: Claude
|
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 (1)
WalkthroughThe PR removes ChangesMethod-form inlining
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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 79ed4a2). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
![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: 79ed4a2862
ℹ️ 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".
Comment on lines
5048
to
5050
| Some((lhs, concrete_lhs, w_class, version_tag)), | ||
| Some((rhs, concrete_rhs, w_typ_r.as_ptr())), | ||
| false, |
There was a problem hiding this comment.
With the method-attribute gate removed here (and in the compare twin), a dunder such as A.__add__(self, other): return self.x is now inlined, but the reflected-method decision was made using the RHS subtype relationship while arg_class_guard only pins the RHS object's physical type and w_class (lines 3072-3104), not that class's version tag. If a compatible RHS class changes __bases__ after the trace becomes hot, its objects retain the same w_class, so the trace keeps calling A.__add__ even when the interpreter would now prioritize the RHS subclass's __radd__; the same issue affects rich comparisons. Pin the RHS type version/MRO dependency before admitting these newly widened bodies.
Useful? React with 👍 / 👎.
youknowone
added a commit
that referenced
this pull request
Aug 2, 2026
…d closure_freevar_branch_resume Six synthetic fixtures landed without a `.jitstats` file, so check.py's always-on regression floor hard-failed each of them with "no committed jit-stats baseline" on every backend it ran: hot_loop_exit_then_class_stmt, raise_reg_unbound_jitstress (#890) inline_freevar_after_mayforce, math_log_trig_hot, tuple_unpack_array_backed_hot (#934) pypy_dict_primitives_nonbinding (#957) Record all three backends for each. A native-only record desynchronizes the tracked `*.wasm.jitstats` set, because `--backend wasm` is not in the default backend list and so would keep comparing against nothing. `closure_freevar_branch_resume` is re-recorded deliberately. Its committed baseline is `loops_aborted=3 loops_compiled=6 guard_failures=23380 bridges_compiled=0`; it now measures `0 / 3 / 606 / 3`, identically on both native backends. The compiled-unit total is unchanged at 6 — three units that were compiled as separate loops are now compiled as bridges — while the aborts disappear and the guard failures fall 38x. Every gated field moves the tightening way (`loops_aborted` and `guard_failures` down, `loops_compiled` down against a fall-gate that now pins 3 instead of 6), so the re-record arms the floor further rather than disarming it. Only `bridges_compiled` rises, and it is in no gated group. Deliberately NOT re-recorded, so the floor keeps reporting them: exception_args_virtual loops_aborted 0 -> 3 exception_multi_handler_warmup loops_aborted 0 -> 23 exception_reraise_tb_depth_jitstress loops_aborted 0 -> 1198, loops_compiled 805 -> 305 list_length_hint_validate loops_aborted 14 -> 34 These raise a `JITSTATS_BADNESS_FIELDS` counter, whose healthy value is 0. Recording them would switch the floor off for those fixtures permanently, which is the opposite of what the counter is for. They are main's own drift away from the baselines `#947` recorded: every one reproduces on both native backends with byte-identical counters, so the movement is walker-level rather than backend-level. Attribution did not converge on a single commit — the nine commits main landed after `#947` include three whose pre-merge check.py was fully green, and reverting `#954` reproduces the counters exactly. `pickle_terminal_raise_resume` (#845) also has no baseline, and is not recorded here either: it does not fail the floor, it crashes (exit 1) on both native backends. The unpickler's read position desyncs under the low JIT thresholds the fixture sets, dispatching `readline()` payload bytes as opcodes. `PYRE_JIT=0` and `pypy3` both pass, so a baseline would only record the crash. Assisted-by: Claude
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two commits. The first is a regression fix for something already on
main.jit: key the raise decline on widened_method_form, not allow_method_load_attrThis fixes a regression currently on
main. #942 was squash-merged with three of its four commits: the widening landed, the commit that scoped its decline did not.maintoday readsFive entries pass
allow_method_load_attr, and four of them — thetype.__call____init__fold, the exception__str__/__repr__override, and the property getter and setter — passed it before the widening. So the decline also withdraws inlines that were already happening. A method-form body with no attribute read hasmethod_form_callee_body_supported == true, hencewidened_method_form == false, and is admitted again.min of 7 interleaved runs, startup subtracted:
maintodayNone of the 358 synthetic benches cover this shape.
jit: inline a user binop/compare dunder whose body reads self.attrtry_walker_inline_user_binopandtry_walker_inline_user_compareopwere the last two entries passingallow_method_load_attr = false, somethod_form_callee_body_supporteddeclined any dunder body carrying aLoadAttrresidual —def __lt__(self, o): return self.x < o.x, the ordinary shape.With both flipped, all seven
try_walker_inline_resolved_user_callcall sites pass the same value, so the parameter and the branch it selected are gone;widened_method_formdrops its now-constant conjunct.There is no upstream counterpart to the check being removed.
rpython/jit/codewriter/policy.py:35look_inside_functionreturnsTrueby default;_reject_function(:38-46) rejects only elidable functions andrpython.rtyper.module.*helpers;look_inside_graph(:48-64) rejects only loop-bearing graphs,_jit_look_inside_overrides and unsupported variable types — all on RPython graphs, never on an app-level body. App-level dunders dispatch uniformly throughpypy/objspace/descroperation.py:706. The gate was a pyre-only deviation.4,000,000 iterations of a
whileloop, min of 5 interleaved runs, startup subtracted, against a binary built fromorigin/main:def __lt__(self, o): return self.x < o.xdef __add__(self, o): return self.x + o.xThe
forform is unchanged (0.99x / 1.01x). The FOR_ITERDeferredCalladmission deniesarg_class_guard.is_some(), which is exactly these two entries, and that denial is what keeps theBINARY_OPabort-rewind from resuming one operand short. Left alone.Semantics
Eight probes in
whileform, 200000 iterations each, matching CPython on both backends and on anorigin/mainbinary:NotImplementedfalling through to the reflected operandAttributeErrorraised inside the body escaping asAttributeError, with theTypeErrorcounter at 0 (no confusion with theNotImplementedpath)__getattr__fallback inside the bodyValueErrorraised once well after the trace is hotVerification
check.py --backend dynasm358/358check.py --backend cranelift358/358cargo test --all --no-default-features7332 passed, 0 failed (pyre-object's SIGABRT is the known multithread GC flake; 3/3 clean on rerun)— authored by Claude
Summary by CodeRabbit