check.py: derive the pypy ratio floor from the ceiling, and arm it only on measurable baselines - #1080
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 249 files, which is 149 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (249)
You can disable this status message by setting the 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 07c4268). 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
|
…-ratio The floor a bench must not read below is now min(1.0, ceiling / 25) for every bench and is stated nowhere else: parity with pypy, dropping proportionally once the ceiling itself comes within 25x of parity. The `# pyre-check: min-pypy-ratio` header is deleted from the 240 fixtures that carried one and is rejected by synth_perf_gate rather than ignored; `run_bench` loses its `min_pypy_ratio` parameter, whose only caller was spectral_norm. The header notes that explained a hand-set floor state only the ceiling's observation now, and two of them lose a leftover duplicate line. Assisted-by: Claude
The floor gate needed the pypy baseline to clear three times EXEC_TIME_FLOOR_S. On windows that is three scheduler ticks, and each side of the ratio carries +-1 tick, so the verdict was read off a number with a third of its magnitude in quantization error: run 31071924467 failed synth/comprehension_accumulators for reading 0.3x against a 0.05s pypy baseline, where macos read the same fixture at 1.6x and 2.5x. At ten times the floor the gate stays armed on 24 fixtures across the three runners, and the widest gap between a ceiling and a ratio reported for one of them is 6x. Assisted-by: Claude
…dir_fd `run.py` passes a script when its last non-empty stdout line is `OK`. dict_subscript_fold, exception_instance_dict_attr and object_init_text_signature end on an assert, so all three interpreters reported `rc=0 last=''` and the row read `cpython=FAIL dynasm=FAIL cranelift=FAIL`. A zero exit with no OK line now says so in the detail instead of looking like an interpreter that produced nothing. os_stat_file_descriptor is marked `platforms=linux,darwin`: windows has no `dir_fd`, so the reference raises NotImplementedError where the fixture pins the ValueError that precedes it. Assisted-by: Claude
![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: 415b3dc859
ℹ️ 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".
| @@ -1,3 +1,8 @@ | |||
| # pyre-check: platforms=linux,darwin | |||
There was a problem hiding this comment.
On win32, _scripts() now honors this marker by excluding the entire fixture, not only the unsupported dir_fd assertions. This also removes the basic os.stat(fd)/os.fstat(fd) checks even though the adjacent comment records that pyre currently raises EBADF for os.stat(fd) there, so a known user-visible Windows parity defect becomes permanently green. Split or conditionally branch the dir_fd cases while continuing to run the descriptor checks on Windows rather than gating the root-cause failure away.
AGENTS.md reference: AGENTS.md:L252-L254
Useful? React with 👍 / 👎.
…it-stat `class_reassign_hot` reads 49.3x on the linux runner's cranelift against a ceiling of 47, and `foriter_inplace_immutable` reads 16.9x against 15. Both ceilings were fitted to a single run's numbers, from 218 and 36; each is now twice the slowest ratio the runners report. `pickle_terminal_raise_resume` compiles 5 fewer loops (72 -> 67 on wasm) since `io.BytesIO` moved to interpreter level: the fixture drives the pure-Python `pickle._dumps`/`_loads`, whose buffer loops are no longer Python code to compile. The linux and windows runners and this machine all report the same counts. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 6, 2026
…can use it `typeobject.py:85` gives the MethodCache ONE `names` array and `text_w` (`unicodeobject.py:133-134`) hands it raw `_utf8` bytes, so upstream has a single key type and no surrogate branch. pyre split the two: `lookup_in_type_where` cached `&str` names while every lone-surrogate name went to `lookup_in_type_wtf8`, an uncached MRO walk that minted a fresh `W_UnicodeObject` per probed class. `MethodCache.names` is now `Vec<Option<Wtf8Buf>>` and `method_hash` takes `&Wtf8` with its body unchanged — `Wtf8::as_bytes` yields the identical bytes, so every existing ascii key lands in the identical slot. `lookup_in_type_where(&str)` becomes a one-line `Wtf8::new` wrapper over the new `lookup_in_type_where_wtf8`, so there is one front door rather than two bodies. `_cached_lookup_where` reads its name through `w_str_get_wtf8` instead of `w_str_get_value`, which also removes the documented panic on a lone surrogate. The residual ABI of `_pure_lookup_where_with_method_cache` is unchanged. `lookup_in_type_wtf8` becomes `lookup_where_wtf8`, returning `(w_class, w_value)` from ONE pass as `typeobject.py:491-501 _lookup_where_all_typeobjects` does — the `&str` `lookup_where_pair` walks the MRO twice only because its two halves are single-register residuals, and that shape is deliberately not copied. Misses funnel through the same `.unwrap_or((null, null))` fill, so a surrogate negative is cached exactly like an ascii one. Only the two TYPE-receiver call sites are rerouted to the cached front door. The three instance-receiver sites keep the uncached walk: `lookup_in_type_where` promotes `w_type` unconditionally, and `type(obj)` there can be megamorphic, so that needs its own three-backend jitstats pass. Measured on this tree (dynasm, N=0 vs N=2000, lldb `--auto-continue` hit counts, a two-name surrogate getattr loop): the uncached WTF-8 pair walk is 2 -> 6 (0.002/iter) and `w_str_from_wtf8` is 2301 -> 2318 (0.0085/iter); the lookups moved onto `_cached_lookup_where_name`, +6.0/iter. `_pure_lookup_where_with_method_cache` reads 0 at both N — and an ascii control loop reads 0 too, so the elidable JIT arm is not exercised on this base at all and the per-iteration global-mutex cost that arm would carry does not arise. `loops_compiled=1 loops_aborted=0` on both probes. check.py ratios (dynasm / cranelift / wasm) before the three commits on this branch and after, same host: `synth/type_dict_surrogate` 34.1 / 37.9 / 44.4 -> 7.2 / 9.2 / 6.9; `instance_surrogate_attrs` 19.1 / 21.5 / 23.6 -> 13.4 / 14.6 / 15.8; `surrogate_kwargs` 16.7 / 19.1 / 24.2 -> 14.4 / 15.4 / 18.9. The after run is on base f828557 (#1080), which changes how the pypy floor is derived but not how the ratio is computed. dynasm 389/389, cranelift 389/389, wasm 385/385. Three Python regressions land in `pyre/extra_tests/parity_tests/` — surrogate store / update / delete invalidation, `__bases__` reassignment, and a metaclass whose `mro()` returns a non-type (the permanently `version_tag == 0` case, a live path). Each runs >= 2000 iterations. check.py does not run `parity_tests`, so these do not gate. `type_set_bases` publishes the new version tag at `typedef.rs:11342` before `w_type_set_bases` / `mro_subclasses` at `:11361-11369`; that ordering is pre-existing and untouched here. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 7, 2026
…can use it `typeobject.py:85` gives the MethodCache ONE `names` array and `text_w` (`unicodeobject.py:133-134`) hands it raw `_utf8` bytes, so upstream has a single key type and no surrogate branch. pyre split the two: `lookup_in_type_where` cached `&str` names while every lone-surrogate name went to `lookup_in_type_wtf8`, an uncached MRO walk that minted a fresh `W_UnicodeObject` per probed class. `MethodCache.names` is now `Vec<Option<Wtf8Buf>>` and `method_hash` takes `&Wtf8` with its body unchanged — `Wtf8::as_bytes` yields the identical bytes, so every existing ascii key lands in the identical slot. `lookup_in_type_where(&str)` becomes a one-line `Wtf8::new` wrapper over the new `lookup_in_type_where_wtf8`, so there is one front door rather than two bodies. `_cached_lookup_where` reads its name through `w_str_get_wtf8` instead of `w_str_get_value`, which also removes the documented panic on a lone surrogate. The residual ABI of `_pure_lookup_where_with_method_cache` is unchanged. `lookup_in_type_wtf8` becomes `lookup_where_wtf8`, returning `(w_class, w_value)` from ONE pass as `typeobject.py:491-501 _lookup_where_all_typeobjects` does — the `&str` `lookup_where_pair` walks the MRO twice only because its two halves are single-register residuals, and that shape is deliberately not copied. Misses funnel through the same `.unwrap_or((null, null))` fill, so a surrogate negative is cached exactly like an ascii one. Only the two TYPE-receiver call sites are rerouted to the cached front door. The three instance-receiver sites keep the uncached walk: `lookup_in_type_where` promotes `w_type` unconditionally, and `type(obj)` there can be megamorphic, so that needs its own three-backend jitstats pass. Measured on this tree (dynasm, N=0 vs N=2000, lldb `--auto-continue` hit counts, a two-name surrogate getattr loop): the uncached WTF-8 pair walk is 2 -> 6 (0.002/iter) and `w_str_from_wtf8` is 2301 -> 2318 (0.0085/iter); the lookups moved onto `_cached_lookup_where_name`, +6.0/iter. `_pure_lookup_where_with_method_cache` reads 0 at both N — and an ascii control loop reads 0 too, so the elidable JIT arm is not exercised on this base at all and the per-iteration global-mutex cost that arm would carry does not arise. `loops_compiled=1 loops_aborted=0` on both probes. check.py ratios (dynasm / cranelift / wasm) before the three commits on this branch and after, same host: `synth/type_dict_surrogate` 34.1 / 37.9 / 44.4 -> 7.2 / 9.2 / 6.9; `instance_surrogate_attrs` 19.1 / 21.5 / 23.6 -> 13.4 / 14.6 / 15.8; `surrogate_kwargs` 16.7 / 19.1 / 24.2 -> 14.4 / 15.4 / 18.9. The after run is on base f828557 (#1080), which changes how the pypy floor is derived but not how the ratio is computed. dynasm 389/389, cranelift 389/389, wasm 385/385. Three Python regressions land in `pyre/extra_tests/parity_tests/` — surrogate store / update / delete invalidation, `__bases__` reassignment, and a metaclass whose `mro()` returns a non-type (the permanently `version_tag == 0` case, a live path). Each runs >= 2000 iterations. check.py does not run `parity_tests`, so these do not gate. `type_set_bases` publishes the new version tag at `typedef.rs:11342` before `w_type_set_bases` / `mro_subclasses` at `:11361-11369`; that ordering is pre-existing and untouched here. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 8, 2026
…can use it `typeobject.py:85` gives the MethodCache ONE `names` array and `text_w` (`unicodeobject.py:133-134`) hands it raw `_utf8` bytes, so upstream has a single key type and no surrogate branch. pyre split the two: `lookup_in_type_where` cached `&str` names while every lone-surrogate name went to `lookup_in_type_wtf8`, an uncached MRO walk that minted a fresh `W_UnicodeObject` per probed class. `MethodCache.names` is now `Vec<Option<Wtf8Buf>>` and `method_hash` takes `&Wtf8` with its body unchanged — `Wtf8::as_bytes` yields the identical bytes, so every existing ascii key lands in the identical slot. `lookup_in_type_where(&str)` becomes a one-line `Wtf8::new` wrapper over the new `lookup_in_type_where_wtf8`, so there is one front door rather than two bodies. `_cached_lookup_where` reads its name through `w_str_get_wtf8` instead of `w_str_get_value`, which also removes the documented panic on a lone surrogate. The residual ABI of `_pure_lookup_where_with_method_cache` is unchanged. `lookup_in_type_wtf8` becomes `lookup_where_wtf8`, returning `(w_class, w_value)` from ONE pass as `typeobject.py:491-501 _lookup_where_all_typeobjects` does — the `&str` `lookup_where_pair` walks the MRO twice only because its two halves are single-register residuals, and that shape is deliberately not copied. Misses funnel through the same `.unwrap_or((null, null))` fill, so a surrogate negative is cached exactly like an ascii one. Only the two TYPE-receiver call sites are rerouted to the cached front door. The three instance-receiver sites keep the uncached walk: `lookup_in_type_where` promotes `w_type` unconditionally, and `type(obj)` there can be megamorphic, so that needs its own three-backend jitstats pass. Measured on this tree (dynasm, N=0 vs N=2000, lldb `--auto-continue` hit counts, a two-name surrogate getattr loop): the uncached WTF-8 pair walk is 2 -> 6 (0.002/iter) and `w_str_from_wtf8` is 2301 -> 2318 (0.0085/iter); the lookups moved onto `_cached_lookup_where_name`, +6.0/iter. `_pure_lookup_where_with_method_cache` reads 0 at both N — and an ascii control loop reads 0 too, so the elidable JIT arm is not exercised on this base at all and the per-iteration global-mutex cost that arm would carry does not arise. `loops_compiled=1 loops_aborted=0` on both probes. check.py ratios (dynasm / cranelift / wasm) before the three commits on this branch and after, same host: `synth/type_dict_surrogate` 34.1 / 37.9 / 44.4 -> 7.2 / 9.2 / 6.9; `instance_surrogate_attrs` 19.1 / 21.5 / 23.6 -> 13.4 / 14.6 / 15.8; `surrogate_kwargs` 16.7 / 19.1 / 24.2 -> 14.4 / 15.4 / 18.9. The after run is on base f828557 (#1080), which changes how the pypy floor is derived but not how the ratio is computed. dynasm 389/389, cranelift 389/389, wasm 385/385. Three Python regressions land in `pyre/extra_tests/parity_tests/` — surrogate store / update / delete invalidation, `__bases__` reassignment, and a metaclass whose `mro()` returns a non-type (the permanently `version_tag == 0` case, a live path). Each runs >= 2000 iterations. check.py does not run `parity_tests`, so these do not gate. `type_set_bases` publishes the new version tag at `typedef.rs:11342` before `w_type_set_bases` / `mro_subclasses` at `:11361-11369`; that ordering is pre-existing and untouched here. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 8, 2026
…can use it `typeobject.py:85` gives the MethodCache ONE `names` array and `text_w` (`unicodeobject.py:133-134`) hands it raw `_utf8` bytes, so upstream has a single key type and no surrogate branch. pyre split the two: `lookup_in_type_where` cached `&str` names while every lone-surrogate name went to `lookup_in_type_wtf8`, an uncached MRO walk that minted a fresh `W_UnicodeObject` per probed class. `MethodCache.names` is now `Vec<Option<Wtf8Buf>>` and `method_hash` takes `&Wtf8` with its body unchanged — `Wtf8::as_bytes` yields the identical bytes, so every existing ascii key lands in the identical slot. `lookup_in_type_where(&str)` becomes a one-line `Wtf8::new` wrapper over the new `lookup_in_type_where_wtf8`, so there is one front door rather than two bodies. `_cached_lookup_where` reads its name through `w_str_get_wtf8` instead of `w_str_get_value`, which also removes the documented panic on a lone surrogate. The residual ABI of `_pure_lookup_where_with_method_cache` is unchanged. `lookup_in_type_wtf8` becomes `lookup_where_wtf8`, returning `(w_class, w_value)` from ONE pass as `typeobject.py:491-501 _lookup_where_all_typeobjects` does — the `&str` `lookup_where_pair` walks the MRO twice only because its two halves are single-register residuals, and that shape is deliberately not copied. Misses funnel through the same `.unwrap_or((null, null))` fill, so a surrogate negative is cached exactly like an ascii one. Only the two TYPE-receiver call sites are rerouted to the cached front door. The three instance-receiver sites keep the uncached walk: `lookup_in_type_where` promotes `w_type` unconditionally, and `type(obj)` there can be megamorphic, so that needs its own three-backend jitstats pass. Measured on this tree (dynasm, N=0 vs N=2000, lldb `--auto-continue` hit counts, a two-name surrogate getattr loop): the uncached WTF-8 pair walk is 2 -> 6 (0.002/iter) and `w_str_from_wtf8` is 2301 -> 2318 (0.0085/iter); the lookups moved onto `_cached_lookup_where_name`, +6.0/iter. `_pure_lookup_where_with_method_cache` reads 0 at both N — and an ascii control loop reads 0 too, so the elidable JIT arm is not exercised on this base at all and the per-iteration global-mutex cost that arm would carry does not arise. `loops_compiled=1 loops_aborted=0` on both probes. check.py ratios (dynasm / cranelift / wasm) before the three commits on this branch and after, same host: `synth/type_dict_surrogate` 34.1 / 37.9 / 44.4 -> 7.2 / 9.2 / 6.9; `instance_surrogate_attrs` 19.1 / 21.5 / 23.6 -> 13.4 / 14.6 / 15.8; `surrogate_kwargs` 16.7 / 19.1 / 24.2 -> 14.4 / 15.4 / 18.9. The after run is on base f828557 (#1080), which changes how the pypy floor is derived but not how the ratio is computed. dynasm 389/389, cranelift 389/389, wasm 385/385. Three Python regressions land in `pyre/extra_tests/parity_tests/` — surrogate store / update / delete invalidation, `__bases__` reassignment, and a metaclass whose `mro()` returns a non-type (the permanently `version_tag == 0` case, a live path). Each runs >= 2000 iterations. check.py does not run `parity_tests`, so these do not gate. `type_set_bases` publishes the new version tag at `typedef.rs:11342` before `w_type_set_bases` / `mro_subclasses` at `:11361-11369`; that ordering is pre-existing and untouched here. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 8, 2026
…location cleanups (#1090) * objspace: record the surrogate attribute-error rendering that 3.14 uses `attr_error_wtf8` puts the name between the quotes verbatim, which the CI parity review read as a dropped `%R`. Measured on both references: `getattr(Sub, '\udcfe')` reports the lone surrogate itself on 3.14 and the six-character escape text through `descroperation.py:58`. The comment records the measurement so the rendering is not restored to the repr form. Comment-only change. Assisted-by: Claude * cranelift: emit the pinned-register read only where the frame is addressed The opcode-emission loop refreshed `jf_ptr` with `get_pinned_reg` before every operation, and `sync_ref_root_var` took an already-materialized pointer even though it stores nothing when the variable owns no ref-root slot. `get_pinned_reg` carries `other_side_effects()` and lowers to a `MovFromPReg` that `is_move()` excludes from coalescing, so neither DCE nor regalloc2 removed the unused ones. The refresh now runs for the operations that address the frame; the call-like arms that already mint their own pointer next to the call keep doing so, and `sync_ref_root_var` mints one lazily per straight-line region through a shared cache. Unused `ForceToken` results no longer materialize a pointer at all. On `call_loop_local_function` the emitted CLIF goes from 102 `get_pinned_reg` (54 with no uses) to 34 (none unused); the loop preamble block drops from 87 instructions to 72. Startup-subtracted, interleaved, 9 rounds at N=480000000 on aarch64 macOS, control and variant built back-to-back from the same `build/llbc`: the cranelift loop goes 0.3722s -> 0.3615s and cranelift/dynasm goes 1.037 -> 1.007. No jit-stats row moves: check.py reads 391/391 on dynasm, 391/391 on cranelift and 387/387 on wasm. Assisted-by: Claude * blackhole: share the descr table as &'static [BhDescr] and delete four caller-free resolvers `BlackholeInterpreter::descrs` and `BlackholeInterpBuilder::descrs` were owned `Vec<BhDescr>`, so `acquire_interp` and `clone_context_from` deep-copied the whole table and the sole producer handed it over with `all_descrs().to_vec()`. `blackhole.py:288` binds `builder.descrs` by reference and `:102-103` stores the assembler list itself; `:154` is the only consumer and it only reads. Both fields are now `&'static [BhDescr]` and the three copy sites are plain aliases. The empty default is `&[]`, matching `:280 EMPTY_LIST_I = [] # shared`. The four resolver mutators that were the only reason the field was owned have no callers anywhere in the tree and are deleted: `BlackholeInterpreter::resolve_field_offsets` / `resolve_jitcode_fnaddrs` and `BlackholeInterpBuilder::resolve_jitcode_fnaddrs` / `resolve_field_offsets`. `pyre/pyre-jit/src/call_jit.rs:1023` named one of them in a doc line; that line is removed and `resolve_field_offset` there is now provably unreachable. `setup_jitdrivers_sd`, `install_global_build_descr_pool`, `ALL_DESCRS` and `jitdrivers_sd` are untouched, as are the three descr resolvers (`runtime_bh_descr`, `read_descr`, the nested inline-call handler), which keep three different fallback chains. Two aliasing tests replace assertions that passed vacuously: the builder test in `jitcode_runtime.rs` pins `ptr::eq` against `all_descrs()` for both the builder and an acquired interpreter, and `blackhole.rs`'s `clone_context_from` test now gives its parent a non-empty table before asserting the alias — its previous `assert_eq!(len, len)` compared 0 to 0. This removes 15 full-table copies on the multi-frame blackhole adoption path and 0 copies on every measured benchmark. It is a correctness-neutral allocation cleanup, not a perf-gate lever; no bench row is attached and no jitstats baseline is re-recorded. Assisted-by: Claude * optimizeopt: stop emitting the w_class SetfieldGc that allocation lowering already makes `AbstractStructPtrInfo._force_elements` (info.py:217-225) emits one SETFIELD_GC per entry of `descr.get_all_fielddescrs()`, and `typeptr` is not in that list — which is why `rewrite.py:479-484` can own the type pointer alone. pyre's `all_fielddescrs()` / `gc_fielddescrs()` do include `w_class`, so `force_box_impl`'s two field loops emitted a store the allocation lowering was about to make anyway: on `bench/synth/type_immutable_reject.py` eight allocations each carried two offset-8 class-pointer stores. `w_class_store_is_covered_by_alloc` (info.rs:24-50) skips the emit only when the allocation lowering provably writes the same bytes: the trace field descr is `is_w_class()`, the size descr's `w_class_obj()` is non-zero, the FIRST `is_w_class` entry of `gc_fielddescrs()` — the same `find` the rewriter selects with — agrees on both `offset()` and `field_size()`, and the forced value is a constant `Value::Ref` equal to that class pointer. A reassigned `__class__`, a non-constant value, or a descr-offset disagreement still emits the store. One helper, called from both the VirtualStruct (info.rs:1160) and Virtual (:1211) loops. This is the same "the value already there" elision as `heap.py:88-101`, at the same layer. The eager `w_class` init in `handle_new` moves out of the `OpCode::NewWithVtable` guard (rewrite.rs:1208-1221). `clear_gc_fields` runs for both fixed-size opcodes and its `is_w_class` skip has no opcode guard, so a plain `New` with a non-zero `w_class_obj()` previously received neither the eager store nor the delayed NULL; with the elision above that slot would have been written only by the nursery's zeroing. The vtable store stays under the `NewWithVtable` guard, as `rewrite.py:482`. Also corrects two `malloc_zero_filled` doc comments that claimed production is always `true`; both backends set it to `false` whenever a real collector is installed (cranelift compiler.rs:8303, dynasm runner.rs:1704). Counts on `type_immutable_reject.py` with N=3000, both binaries built from 6073a3b0686 against the same LLBC: `[cl-gcstore]` 272 -> 264, `pre-backend` 141 -> 133 ops, output unchanged at 6000, and every one of the sixteen offset-8 bases ends with exactly one non-zero class store. wasm runs no GC-rewriter pass; its `genop_new_with_vtable` already initialises `w_class` from the size descr, so the invariant holds there too. Assisted-by: Claude * objspace: key the method cache by WTF-8 so surrogate attribute names can use it `typeobject.py:85` gives the MethodCache ONE `names` array and `text_w` (`unicodeobject.py:133-134`) hands it raw `_utf8` bytes, so upstream has a single key type and no surrogate branch. pyre split the two: `lookup_in_type_where` cached `&str` names while every lone-surrogate name went to `lookup_in_type_wtf8`, an uncached MRO walk that minted a fresh `W_UnicodeObject` per probed class. `MethodCache.names` is now `Vec<Option<Wtf8Buf>>` and `method_hash` takes `&Wtf8` with its body unchanged — `Wtf8::as_bytes` yields the identical bytes, so every existing ascii key lands in the identical slot. `lookup_in_type_where(&str)` becomes a one-line `Wtf8::new` wrapper over the new `lookup_in_type_where_wtf8`, so there is one front door rather than two bodies. `_cached_lookup_where` reads its name through `w_str_get_wtf8` instead of `w_str_get_value`, which also removes the documented panic on a lone surrogate. The residual ABI of `_pure_lookup_where_with_method_cache` is unchanged. `lookup_in_type_wtf8` becomes `lookup_where_wtf8`, returning `(w_class, w_value)` from ONE pass as `typeobject.py:491-501 _lookup_where_all_typeobjects` does — the `&str` `lookup_where_pair` walks the MRO twice only because its two halves are single-register residuals, and that shape is deliberately not copied. Misses funnel through the same `.unwrap_or((null, null))` fill, so a surrogate negative is cached exactly like an ascii one. Only the two TYPE-receiver call sites are rerouted to the cached front door. The three instance-receiver sites keep the uncached walk: `lookup_in_type_where` promotes `w_type` unconditionally, and `type(obj)` there can be megamorphic, so that needs its own three-backend jitstats pass. Measured on this tree (dynasm, N=0 vs N=2000, lldb `--auto-continue` hit counts, a two-name surrogate getattr loop): the uncached WTF-8 pair walk is 2 -> 6 (0.002/iter) and `w_str_from_wtf8` is 2301 -> 2318 (0.0085/iter); the lookups moved onto `_cached_lookup_where_name`, +6.0/iter. `_pure_lookup_where_with_method_cache` reads 0 at both N — and an ascii control loop reads 0 too, so the elidable JIT arm is not exercised on this base at all and the per-iteration global-mutex cost that arm would carry does not arise. `loops_compiled=1 loops_aborted=0` on both probes. check.py ratios (dynasm / cranelift / wasm) before the three commits on this branch and after, same host: `synth/type_dict_surrogate` 34.1 / 37.9 / 44.4 -> 7.2 / 9.2 / 6.9; `instance_surrogate_attrs` 19.1 / 21.5 / 23.6 -> 13.4 / 14.6 / 15.8; `surrogate_kwargs` 16.7 / 19.1 / 24.2 -> 14.4 / 15.4 / 18.9. The after run is on base f828557 (#1080), which changes how the pypy floor is derived but not how the ratio is computed. dynasm 389/389, cranelift 389/389, wasm 385/385. Three Python regressions land in `pyre/extra_tests/parity_tests/` — surrogate store / update / delete invalidation, `__bases__` reassignment, and a metaclass whose `mro()` returns a non-type (the permanently `version_tag == 0` case, a live path). Each runs >= 2000 iterations. check.py does not run `parity_tests`, so these do not gate. `type_set_bases` publishes the new version tag at `typedef.rs:11342` before `w_type_set_bases` / `mro_subclasses` at `:11361-11369`; that ordering is pre-existing and untouched here. Assisted-by: Claude * virtualizable_spec: correct the two upstream citations on the last vable scalars `pypy/module/pypyjit/interp_jit.py:25-30` is `['last_instr', 'pycode', 'valuestackdepth', 'locals_cells_stack_w[*]', 'debugdata', 'w_globals']` with the closing `]` on `:30`. The table cited `:30` for `lastblock` — a line that holds no field name — and `:31` for `w_globals`, which is past the end of the literal; `w_globals` is on `:29`. `lastblock` has no `_virtualizable_` entry at all, which the doc block above the table already states, so its comment now says so instead of naming a line. Assisted-by: Claude * _io: report the BytesIO as the owner of the view getbuffer returns `getbuffer` built its memoryview with `w_memoryview_new_with_flags(self.buffer, ...)`, which derives both the backing exporter and the reported `.obj` from its single argument, so `io.BytesIO(b"abc").getbuffer().obj` was the private backing bytearray. `interp_bytesio.py:149-152` keeps the two apart: `BytesIOBuffer(self)` reads the storage while `BytesIOView.__init__` passes `w_obj=w_bytesio` to `SimpleView.__init__` (`:52-62`), so `.obj` is the BytesIO. Add `w_memoryview_new_simple_with_owner`, which takes the backing and the owner separately, and call it from `getbuffer`. The export count stays on the backing: the new entry point increfs the bytearray's `_exports`, and `memoryview_release` decrefs through `w_memoryview_backing`, which reads the view's `Buffer`, not its `w_obj`. `check_exports` is unchanged. pypy3 reads `getbuffer().obj is b` as True; CPython 3.14 reports a private `_io._BytesIOBuffer` instead. Assisted-by: Claude * bench, jit: correct three comments that the recorded evidence contradicts `type_immutable_reject.py:11-13` said the raising STORE_ATTR/DELETE_ATTR makes the JIT "deopt into the blackhole" every iteration. The baselines beside it read `loops_compiled=1 guard_failures=1 bridges_compiled=0` on all three backends — one bailout for 200000 iterations. `try_walker_trace_immutable_type_attr_raise` (specialize.rs:9717) folds the raise into a `NewWithVtable` + `SetfieldGc` construction routed through `SubRaise`, so the raise and its catch are paid inside the compiled loop. The comment now says that. `inline_call.rs` justified the `defs_w` identity `GuardValue` with a "`GuardValue` over an `arraylen_gc` leaves the guard's only argument dead after it" mechanism. The replacement was implemented and reverted: it answers correctly on every defaults shape but segfaults `synth/pickle_terminal_raise_resume` deterministically, and the same build with the class guard kept and this `GuardValue` restored is clean, so the length guard is what is unsound. The dead-argument mechanism is not what the code shows, so the comment now records the measurement instead of the mechanism. The same comment's cost claim ("only costs the shape that builds the callee in the caller's own loop AND omits an argument it has a default for") overstates it. `codegen.py:582-590 _visit_defaults` takes the `_tuple_of_consts` branch for an all-constant defaults list, and pyre's compiler emits the same single `LOAD_CONST (None, 7)` where a non-constant default gets `BUILD_TUPLE` — so a loop-local `def` with literal defaults hands out one code constant and the identity guard never fails. `make_function_inline`, the only loop-local `def` with a default in `bench/`, records `guard_failures=1`. `CalleeReplaySafety::DeferredCall` carried no note that it and the nested-residual abort are one contract; the enforcer at fbw_state.rs:1413 states the promise from the other side only. The variant now records it, that the axis is the executed-effect delta rather than raising, and that `look_inside_graph` (`codewriter/policy.py:48`) and `can_inline_callable` (`warmstate.py:669`) are upstream's static-decision counterparts. Comment-only change. Assisted-by: Claude * jit: drop lastblock from the PyFrame virtualizable scalar table `PYFRAME_VABLE_FIELDS` listed six scalars; `interp_jit.py:25-30` lists five plus one array. `lastblock` was the extra one. `rg lastblock pypy/` returns no hits: the vendored PyPy is 3.11, which has no `PyFrame.lastblock`, no block stack and no `FrameBlock`, and unwinds via `pyopcode.py:152 lookup_exceptiontable`. On pyre's side no production path writes it either — `setup_finally` / `setup_except` / `pop_block` (`pyre-interpreter/src/eval.rs`) are the only `append_block` callers and the 3.14 bytecode emits no SETUP_FINALLY / SETUP_EXCEPT / POP_BLOCK. Removed: - the `("lastblock", 4)` entry; `w_globals` renumbers 5 -> 4, and `VABLE_NAMESPACE_FIELD_IDX` (`jit/codewriter.rs`) follows; - `VABLE_STATIC_FIELD_DESCR_SLOTS` 6 -> 5 (`majit-ir/src/descr.rs`); - the `lastblock: Ref` inputarg and the `lastblock: ref @ PYFRAME_LASTBLOCK_OFFSET` field from `virtualizable_gen.rs`; - `vable_lastblock` and the three walk-end flush gates that compared it against `*(frame_ptr + PYFRAME_LASTBLOCK_OFFSET)` (`flush_walk_end_state_to_frame_inner`, `flush_walk_end_state_at_outer_call`, `can_flush_walk_end_state_after_outer_call`) — with no writer the comparison could not fail; - the unused `pyframe_lastblock_descr()`. Kept: `PYFRAME_LASTBLOCK_OFFSET`, the heap field, its `"PyFrame.lastblock"` entry in `PYFRAME_DESCR_GROUP` and its GC root slot. `flatten_descr_by_ptr` probed `0u16..6` as a literal and panicked with `idx=5 exceeds VABLE_STATIC_FIELD_DESCR_SLOTS=5` on every trace; the bound now reads `NUM_VABLE_SCALARS`. `virtualizable/{mod,derive}.rs` cited `interp_jit.py:25-31`, corrected to `:25-30`. `test_setup_bridge_sym_preserves_resumed_stack_tail` drops the `lastblock` slot from its `fail_values` / `fail_types` fixture and shifts the `RebuiltValue::Box` indices down by one to match. check.py: dynasm 391/391, cranelift 391/391, wasm 387/387, no jitstats baseline moved. `cargo test --all --no-default-features --features dynasm` green. Assisted-by: Claude * jit: emit the virtualizable store-back on the bridge carrier-raise escape `dispatch_via_miframe`'s carrier-raise-escape arm published the exit `last_instr` and terminated with the raise but never emitted `gen_store_back_in_vable`. `record_top_level_application_traceback` performs that write only concretely, for the recording pass, so a compiled bridge left the frame's `locals_cells_stack_w` holding whatever its entry wrote, and a `tb_frame.f_locals` or `sys._getframe()` read on the way out saw every post-entry local as unbound. The walk-level twin in `jitcode_dispatch/mod.rs` already calls `fbw_force_virtualizable_before_return` in the same position. Assisted-by: Claude * build: fail on a stale LLBC artefact instead of warning `warn_if_llbc_stale` becomes `fail_if_llbc_stale` and the `PYRE_LLBC_STRICT` gate is inverted: a fingerprint mismatch now goes out as `cargo::error` and exits 1 by default, and `PYRE_LLBC_STRICT=0` selects the previous `cargo::warning`. `PYRE_LLBC_SKIP_FINGERPRINT_CHECK` still skips the comparison entirely. Assisted-by: Claude * jit: stop lowering a by-value substructure read to a load A field whose type is an inlined by-value substructure owns no flattened FieldDescr, so `fielddescrof` fell through to the raw struct-layout row and emitted `getfield_gc_r(base, offsetof(sub))` — an 8-byte load at the address `&x.sub` should have produced. `rewrite_op_getsubstruct` (`jtransform.py:942-950`) emits `int_add(base, offsetof)` there and refuses outright when the structure is GC-managed (`:945-946`). `inline_substruct_field_offset` shares `fielddescrof`'s lookup order and reports that shape. `rewrite_op_getfield` now aliases the read to its base at offset zero, and prepends a result-less `abort/` when the offset is nonzero and the result is consumed as a call argument — the shape whose callee dereferences the value as a raw pointer. The abort carries no result because `abort/>r` and `abort/>i` only advance the pc. `import random` plus `for i in range(20000): random.random()` exited 139 on 6 of 6 runs and now exits 0; `lib-python/3/test/test_float.py` did the same. Assisted-by: Claude * typedef: stamp the metatype on builtin type objects built outside the registry `w_type_new_builtin` allocates with `w_class` null, and the sweep at the end of `init_typeobjects` fills the slot only for the types held in `TYPEOBJECT_CACHE`. Four builder families construct builtin type objects that never enter that registry — `getset_descriptor_type()`, `make_exc_class`, `posix.DirEntry`, and the `py_class_typed!` / `#[pyre_class]` natives — so those type objects kept a null `w_class`. `typedef::r#type` falls back to `gettypefor(ob_type)`, so the null is not visible from Python. Route the three `w_type_new_builtin` call sites through `new_builtin_typeobject`, which stamps `w_class = w_type()`, and chain `GETSET_DESCRIPTOR_TYPE` into the sweep for the one type built before the `type` typeobject is published. A hot `Cls.__name__` loop over 37 types read 10 of them at bridges_compiled=7 / guard_failures=1480 where the registry-resident ones read 0 / 1; after the change all 37 read 0 / 1. `synth/pypy_type_surface` moves from bridges_compiled=102 guard_failures=20498 to bridges_compiled=5 guard_failures=1012 against a recorded 5 / 1011, and `pyre/check.py` reports dynasm 405/405, cranelift 405/405, wasm 401/401. 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.
pyre/check.py's pypy performance gate has a floor as well as a ceiling. Thefloor was a per-fixture number: 240 synthetic fixtures carried a
# pyre-check: min-pypy-ratio=header, andrun_benchtook amin_pypy_ratiokeyword. Each of those numbers asserts that a fixture must stay some number
of times slower than pypy, which is not a thing anyone wants to be true, and
each has to be re-fitted every time its ceiling moves.
The floor is now derived and stated nowhere else:
Parity with pypy is what every bench is asked for, so parity is the floor; it
drops proportionally only once the ceiling itself comes within 25x of parity,
where a floor at parity would sit inside the band the ceiling already allows.
synth_perf_gaterejects a leftovermin-pypy-ratioheader rather thanignoring it.
The floor was arming on baselines too small to read
Run 31071924467 failed
synth/comprehension_accumulatorson windows forreading 0.3x — against a pypy baseline of 0.05s. On windows that is 3.2
scheduler ticks, each side of the ratio carries ±1 tick, and the same fixture
read 1.6x and 2.5x on macos in the same run. The gate armed at
3 * EXEC_TIME_FLOOR_S, so it was reading a "you got fast" verdict off anumber with a third of its own magnitude in quantization error.
FLOOR_GATE_MIN_BASELINE_Sis now10 * EXEC_TIME_FLOOR_S. Across the threerunners in that run, the gate then stays armed on 24 fixtures whose pypy time
is real work, and the widest gap between a ceiling and a ratio reported for one
of them is 6x — comfortably inside the divisor.
Verification
Every fixture whose header note changed, plus every fixture that still arms the
floor, run green on dynasm and cranelift locally. No fixture's floor moves
tighter than the ratio any runner has reported for it.
— opened by Claude
Rebased onto #1079, and the parity_tests failures that came with it
The gate step is green on all three runners at the previous head —
dynasm 388/388,cranelift 388/388, ubuntu alsowasm 384/384, windowsincluded. What was left red was
parity_tests, and none of it was thischange:
dict_subscript_fold,exception_instance_dict_attrandobject_init_text_signature(all added by mapdict, dict, inline: instance-dict backing, live exact-dict lookups, and a materialized callee frame's locals #1067) end on an assert and neverprint the final
OKlinerun.pyrequires, so all three interpretersreported
rc=0 last=''and the row readcpython=FAIL dynasm=FAIL cranelift=FAIL. Each gets itsprint("OK"), andrun.pynow says "exited 0without a final 'OK' line" instead of a detail that reads like an interpreter
which produced nothing.
os_stat_file_descriptorisplatforms=linux,darwin: windows has nodir_fd, so the reference raisesNotImplementedErrorwhere the fixture pinsthe
ValueErrorthat precedes it. This is the first run in which the windowsjob ever reached
parity_tests— the gate step used to stop it.python3 pyre/extra_tests/parity_tests/run.pyreportsall parity tests passlocally on both backends, rebuilt at this base.