interp: admit method_descriptor in the LOAD_ATTR-method fold precondition - #896
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesDescriptor binding compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
✨ 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 9d5e468). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
…tion `bound_method_attr_fast_path` gates on `(*w_descr).ob_type == FUNCTION_TYPE`. The `get()` it reproduces (`baseobjspace.rs:9516-9527`) takes the same arm for `FUNCTION_TYPE` and `METHOD_DESCRIPTOR_TYPE` — both with builtin code bind through `w_method_new` — and `is_function` already covers both, so the predicate was narrower than the behaviour it mirrors. Every `TypeDef` method is retagged `method_descriptor` (`function_retag_method_descriptor`), so the gate declined `lst.append`, `d.get`, `s.add` and every sibling — the shape `try_walker_specialize_load_bound_method_attr` was written for and names in its own doc. The walker fell through to the `bh_load_attr_fn` residual on every iteration: `CALL_MAY_FORCE` + `GUARD_NOT_FORCED` (forcing the virtualizable frame) + `GUARD_NO_EXCEPTION`, plus the `Method` allocation the fold virtualizes away. `BUILTIN_FUNCTION_TYPE` stays excluded: `get()` returns it unbound, so there is no `Method` to emit. A/B at base 8c87851 with the LLBC corpus re-extracted for it; user+sys CPU exec (check.py's metric), min of 3, dynasm vs pypy: A (base) B (this fix) r = []; for i in ...: r.append(i) 1.6863s 56.1x 0.2428s 7.8x synth/const_arg_call_resume k=12800 2.4903s 34.0x 1.1180s 16.0x CallMayForceR in the optimized loop 49 0 The `Method` allocation is gone from the trace too — the optimizer virtualizes it once the fold emits it as New+SetField rather than a residual result. `bound_method_fast_path_admits_the_same_kinds_get_binds` pins the predicate against `get()` on `list.append`, asserting the premise (a TypeDef method is a `method_descriptor`) so it fails rather than passing vacuously if the retagging changes. Assisted-by: Claude
d8812d6 to
9d5e468
Compare
bound_method_attr_fast_path— the precondition fortry_walker_specialize_load_bound_method_attr— gated on:The
get()it reproduces (baseobjspace.rs:9516-9527) takes the same arm forFUNCTION_TYPEandMETHOD_DESCRIPTOR_TYPE— both with builtin code bind throughw_method_new— andis_function()already covers both. So the predicate wasnarrower than the behaviour it mirrors.
Every
TypeDefmethod is retaggedmethod_descriptor(
function_retag_method_descriptor,function.rs:541), so the gate declinedlst.append,d.get,s.add,str.find,lst.pop— every builtin method, i.e.exactly the shape the fold was written for and names in its own doc. The walker
fell through to the
bh_load_attr_fnresidual on every iteration:CALL_MAY_FORCEforces the virtualizable frame throughGUARD_NOT_FORCEDeveryiteration, and the
Methodthe fold would have virtualized away is allocated forreal.
BUILTIN_FUNCTION_TYPEstays excluded:get()returns it unbound, so there is noMethodto emit.Why the PyPy reading misleads here
type(list.append)ismethod_descriptorin pyre andfunctionin PyPy — soreading upstream for the shape suggests the
FUNCTION_TYPEgate is right. Pyre'sspelling is the correct one for the 3.14 target and PyPy's
functionis thePyPy-ism, so the fix is to widen the predicate, not to retype
list.append.How it was localized
MAJIT_STATSfor the append loop and the same loop written as a comprehension werebyte-identical —
loops_compiled=3 bridges_compiled=3 loops_aborted=3 guard_failures=11742for both — while exec time differed 8x. That rules out tracecount, aborts and bridge churn in one step. The optimized-trace diff was the 9 ops
above. The residual was identified by matching
CallMayForceR(addr, v9, ptr(const), 1)witharg_types: [Ref, Ref, Int] -> Refagainstbh_load_attr_fn(obj, w_code_ptr, name_idx)(call_jit.rs:4871): v9 = theloop-invariant list, const = the code object, 1 = the
co_namesindex of"append".Measurement
A/B at base
8c878513efwith the LLBC corpus re-extracted for it — user+sys CPUexecution time (check.py's own metric), min of 3, dynasm vs pypy:
r = []; for i in range(1000): r.append(i)synth/const_arg_call_resume(k=12800)CallMayForceRin the optimized loop6.9x on the target form, 2.2x on the bench. The
Methodallocation is gone fromthe trace as well — the optimizer virtualizes it once the fold emits it as
New+SetField instead of a residual result.
Attribution note: #878 ("inline bound methods and defaults") landed on main while
this was in progress. It is complementary, not overlapping — its
try_walker_inline_builtin_call(inline_call.rs:1994) unwraps aMethodcallable into (function, receiver) and inlines the CALL half, and it consumes
the virtual
Methodthis fold emits. Measured on its own it moves the append loop63.9x → 56.1x and leaves the residual intact; the LOAD_ATTR half is this
commit's.
Verification
At base
8c878513ef, corpus freshly extracted, both backends rebuilt:pyre/check.py --backend dynasm,cranelift— 342/342 and 342/342, exit 0.cargo test -p pyre-interpreter --features dynasm— 432 passed, 0 failed.cargo test -p pyre-object— 282 passed, 0 failed.list.append/dict.get/set.add/str.find/list.popload:JIT output identical to
PYRE_NO_JIT=1, pypy and cpython.const_arg_call_resume.dynasm.jitstatsis unchanged against the committedbaseline (
loops_compiled=3 bridges_compiled=9 loops_aborted=0 guard_failures=1804) — the win is per-iteration cost, not fewer guard failures,so no baseline re-record.
The branch has since been replayed onto
fdca72a7fe, which adds only #891 (alreadyverified green with and without on the previous base); the numbers above are from
8c878513ef.Side effect on the perf gate
synth/const_arg_call_resume'smax-pypy-ratio=30was calibrated against acollapsed denominator: at the committed
k=400pypy's execution time is 0.0061s,barely above
EXEC_TIME_FLOOR_S = 0.005, and the ratio converged upward to36x/41x as the workload grew — which is why CI reported 41.2x/45.7x while local runs
passed at 22.8x. With this commit k=12800 reads 16.0x, so sizing that bench to an
honest denominator no longer needs the gate moved.
Remaining levers in the same loop (not in this commit)
w_classandversion_tagGuardValues are re-emitted everyiteration although the receiver is loop-invariant — not hoisted to the preamble
the way upstream's
@elidable_promoteversion_tag read is.NewWithVtable W_IntObject+SetfieldGc intvalfollowed immediately byGetfieldGcI W_IntObject.intval— box then unbox, the allocation surviving eventhough the box is dead after the int-strategy store. All three bench forms pay
it (7.8x / 15.2x / 20.4x).
PtrEq/IntIsTrue/Guardchains plusGetfieldGcR PyType.instantiate—per-iteration strategy/type dispatch after the strategy is already guarded.
🤖 Generated with Claude Code
Summary by CodeRabbit
get().get()and fast-path method binding, ensuring methods such aslist.appendbind correctly.