interp: port the StdObjSpace finditem dict shortcut; jit: trace_limit in the single walker - #789
Conversation
The module is built for wasm32-unknown-unknown, whose `std::env` is always empty, so every `std::env::var_os` reached from inside the guest returns None regardless of this process's environment. Every JIT knob and probe that lives in the guest crates is therefore inert under the wasm runner: `PYRE_NO_JIT`, `PYRE_GC_INTERP`, `MAJIT_STRICT`, `MAJIT_LOG`, and the `PYRE_FBW_*` / `PYRE_P2_DIAG` / `PYRE_JD1*` diagnostics among them. Nothing reported this, so setting one produced a run indistinguishable from not setting it — an A/B through such a knob measures the same build twice. `warn_inert_guest_env` names them once on stderr at startup. It matches the `PYRE_` / `MAJIT_` prefixes and exempts the names interpreted host-side (`PYRE_WASM_*`, `PYRE_STDLIB`, `MAJIT_STATS`) plus `check.py`'s `PYRE_CHECK_*` interpreter paths, so new guest-side knobs need no upkeep here. check.py sets MAJIT_STRICT=1 in `pyre_env()`, so its wasm leg reports that name on every run; it shows a run's stderr only on failure, and the counters `_jit_stats_snapshot` parses are unaffected. Assisted-by: Claude
RPython `pyjitpl.py:2865 `_interpret`` calls `blackhole_if_trace_too_long()` after every `run_one_step()`, aborting a trace once `history.length()` passes `warmstate.trace_limit` (`rlib/jit.py:592`, 6000). `jitcode_dispatch::walk` is that loop's counterpart — `step` is `run_one_step` — and carried no such check: the per-opcode tracing loop it replaced ran it (`pyre-jit/src/eval.rs:6301` still does on that path), and `state.rs`'s `note_root_trace_too_long` has been dead since. So a walk that kept recording without reaching a close had no bound. `synth/gc_iterator_source_drop` on wasm grew a single bridge trace to 21038 ops and re-ran the full optimizer at each of 252 merge points, 13.5s inside one `compile_and_run_once`, with `loops_aborted` reading 0 throughout. `TraceCtx::is_too_long` is the `history.length() > trace_limit` test and `num_ops` is a `Vec::len`, so this is one comparison per step. On overflow the walk calls `note_root_trace_too_long` (the warm-state half the retired loop called: `trace_next_iteration` + `mark_force_finish_tracing`, so the next attempt cuts the trace instead of growing it again) and returns a new `DispatchError::TraceTooLong`, which the existing fall-through maps to `TraceAction::Abort` and the census records by name. The `find_biggest_function` -> `disable_noninlinable_function` half (pyjitpl.py:2793) still runs only on the per-opcode path. check.py: dynasm 310/310, cranelift 310/310, wasm 307/307. No bench's `loops_aborted` moved on the native backends. On wasm `synth/gc_iterator_source_drop` goes 11.80s -> 4.89s with `loops_aborted` 0 -> 5 and `compile_ms` 65.0 -> 9.4: the runaway trace is now cut instead of compiled. Its remaining gap to dynasm is the wasm backend declining the entry bridge's cross-loop JUMP, which is what makes that walk run away. Assisted-by: Claude
`JITSTATS_BADNESS_FIELDS`'s comment claimed both counters "read 0 in every healthy baseline on every platform", and `_apply_snapshot_gate` repeated it. That is true of `internal_compile_panics` but not of `loops_aborted`: the nine committed baselines are the only `.jitstats` fixtures in the tree and all carry 1-2 aborts. Each of those aborts is `LoopBearingCalleeInlineUnsupported` -- the walker's open gap on inlining a callee whose body is not a straight-line leaf (`jitcode_dispatch/mod.rs:1758-1770`). The counts line up 1:1 with the census under `PYRE_FBW_CENSUS=1`: comprehension_object_append_hot 2 declines / loops_aborted=2 const_arg_call_resume 1 decline / loops_aborted=1 nested_list_comprehension_hot 2 declines / loops_aborted=2 The gate itself is a ratchet on the committed value, so a nonzero baseline was already what it was built for. Restate the two comments accordingly: the field is gated because its direction is stable, the baseline pins the count today's known declines produce, and a rise means a loop that used to compile stopped compiling. Assisted-by: Claude
`finditem` implemented only `baseobjspace.py:873`'s generic form -- call `getitem`, catch `KeyError`, return None. `objspace.py:745-766` overrides both it and `finditem_str` for a plain dict with what upstream calls a "performance shortcut to avoid creating the OperationError(KeyError) and allocating W_BytesObject", answering the probe through the receiver's own strategy slot. That override was missing here, so every dict miss built a full exception object -- key `repr` included -- and threw it away. Attribute lookup takes that route on paths where a miss is the ordinary result. A guest profile of `while i < N: import math` attributes 54.9% of all samples to `PyError::key_error_with_key`, reached three ways, all from `gcd_import_fast`: getattr_str_impl -> module_getattr_hook_or_err -> getitem (2330 samples) getattr_str_impl -> object_getattr_miss -> getitem (1194) getattr_str_impl -> getitem (1117) Port both overrides. The receiver gate is upstream's `isinstance(w_obj, W_DictMultiObject) and not w_obj.user_overridden_class`: `is_dict` answers the user-visible isinstance question, so a module dict passes, and the exact-class comparison keeps a dict subclass -- whose `__getitem__` or `__missing__` may override the probe -- on the generic path. `finditem_str` probes with the borrowed `&str`, dropping the wrapped key as well as the exception. `finditem`'s shortcut is the dict arm of `getitem_slot` minus its `dict_missing_or_key_error` branch, which only an excluded subclass can reach, and recovers a raising `__hash__`/`__eq__` through the same `take_pending_dict_key_error`. Interleaved A/B against a baseline binary, min of 7, and the check.py suite: synth/import_from_hot dynasm 1.49s -> 0.77s wasm 2.50s -> 0.60s synth/import_name dynasm 0.28s -> 0.23s wasm 2.10s -> 0.49s The wasm share is larger because the discarded exception is an allocation, and allocation carries wasm's constant factor. check.py: dynasm 312/312, cranelift 312/312, wasm 309/309. Assisted-by: Claude
WalkthroughThe changes clarify JIT-stats gating documentation, optimize exact-dict lookups, report trace-length exhaustion during JIT walks, and warn about environment variables unavailable to the wasm guest. ChangesJIT-stats gating documentation
Dictionary lookup shortcuts
Trace-length dispatch reporting
Wasm environment warnings
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 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 559338c). 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
|
![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: 559338c3c7
ℹ️ 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".
| return Err(DispatchError::TraceTooLong { | ||
| pc: opcode_position, | ||
| ops, | ||
| }); |
There was a problem hiding this comment.
When a full-body walk exceeds the limit after executing a non-journaled concrete residual call, returning an ordinary DispatchError sends it through run_perfn_walk's uncommitted-error epilogue and the generic TraceAction::Abort replay path. The walk uses a private frame snapshot but shares the heap, so effects such as a method mutating an object remain applied and are then executed again when interpretation restarts from the trace entry (and FOR_ITER paths can instead drop the consumed iteration). RPython's SwitchToBlackhole(ABORT_TOO_LONG) resumes from the current traced state rather than treating this as an arbitrary walker decline; this path needs the equivalent forward blackhole/adoption handoff before it can safely abort at any opcode.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
| // (pyjitpl.py:2793) still only runs on the per-opcode path. | ||
| if ctx.trace_ctx.is_too_long() { | ||
| let ops = ctx.trace_ctx.num_recorded_ops(); | ||
| crate::state::note_root_trace_too_long(ctx.trace_ctx.root_green_key()); |
There was a problem hiding this comment.
When the oversized walk is a bridge (ctx.is_bridge_trace), this unconditionally marks the root green key with trace_next_iteration/mark_force_finish_tracing. Bridges have no outer merge-point key to mark: rpython/jit/metainterp/pyjitpl.py:2814-2833 instead sets FORCE_BRIDGE_SEGMENTING on the source JitCellToken, and the existing MetaInterp::prepare_trace_segmenting implementation already preserves that distinction. Without setting that token bit, subsequent failures of the same guard can repeatedly record and abort the same oversized bridge while the unrelated root cell is modified.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
| if is_shortcut_dict(obj) { | ||
| return Ok(unsafe { pyre_object::dictmultiobject::w_dict_getitem_str(obj, key) }); |
There was a problem hiding this comment.
For an exact dict in object-strategy mode containing a non-string key whose hash collides with key, the borrowed probe can invoke that stored key's user-defined __eq__. If it raises, dict_keys_equal records the pending error and returns false, but w_dict_getitem_str exposes only Option, so this new Ok(...) wrapper reports a miss and leaves the exception undrained. The previous path wrapped the string and reached the new fallible w_dict_lookup_checked, which propagates the error; name/global lookups using such a dict must likewise return the pending PyError rather than silently continuing as though the name were absent.
AGENTS.md reference: AGENTS.md:L194-L196
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/mod.rs`:
- Around line 2027-2050: After updating the TraceTooLong handling around
note_root_trace_too_long, re-extract the corresponding Charon .ullbc files
before rebuilding the rtyper prepass, then run all eight benchmarks and verify
they pass without regressions.
In `@pyre/pyre-wasm-runner/src/main.rs`:
- Around line 129-130: Update the environment-variable collection in the inert
settings initialization to preserve non-UTF-8 names instead of dropping them via
into_string().ok(). Convert names lossily or retain OsString values through the
PYRE_ and MAJIT_ filtering so malformed names remain represented while still
being ignored by the guest.
🪄 Autofix (Beta)
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: 291f72c9-bd5f-47dd-9cc2-8a279e4593c5
📒 Files selected for processing (4)
pyre/check.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-wasm-runner/src/main.rs
| // pyjitpl.py:2865 `_interpret`: `blackhole_if_trace_too_long()` runs | ||
| // after every `run_one_step()`. This loop is that loop's counterpart — | ||
| // `step` is `run_one_step` — and the check came with the per-opcode | ||
| // tracing loop it replaced (`pyre-jit/src/eval.rs` still runs it on | ||
| // that path), so nothing bounded a walk that kept recording without | ||
| // reaching a close. `TraceCtx::is_too_long` is the `history.length() > | ||
| // warmrunnerstate.trace_limit` test and `num_ops` is a `Vec::len`, so | ||
| // this is one comparison per step. | ||
| // | ||
| // The walker layer holds `&mut TraceCtx` and cannot reach | ||
| // `MetaInterp::blackhole_if_trace_too_long` for the full bookkeeping; | ||
| // `note_root_trace_too_long` is the warm-state half the retired loop | ||
| // called (`trace_next_iteration` + `mark_force_finish_tracing`, so the | ||
| // next attempt cuts the trace instead of growing it again). The | ||
| // `find_biggest_function` → `disable_noninlinable_function` half | ||
| // (pyjitpl.py:2793) still only runs on the per-opcode path. | ||
| if ctx.trace_ctx.is_too_long() { | ||
| let ops = ctx.trace_ctx.num_recorded_ops(); | ||
| crate::state::note_root_trace_too_long(ctx.trace_ctx.root_green_key()); | ||
| return Err(DispatchError::TraceTooLong { | ||
| pc: opcode_position, | ||
| ops, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
Verify the required post-JIT validation.
Please confirm that the corresponding Charon .ullbc files were re-extracted before rebuilding the rtyper prepass and that all eight benchmarks passed without regression.
As per coding guidelines, these checks are required after JIT changes.
🤖 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/mod.rs` around lines 2027 - 2050,
After updating the TraceTooLong handling around note_root_trace_too_long,
re-extract the corresponding Charon .ullbc files before rebuilding the rtyper
prepass, then run all eight benchmarks and verify they pass without regressions.
Source: Coding guidelines
| let mut inert: Vec<String> = std::env::vars_os() | ||
| .filter_map(|(name, _)| name.into_string().ok()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -C3 'vars_os\(\)|into_string\(\)\.ok\(\)|to_string_lossy\(\)' \
pyre/pyre-wasm-runner/src/main.rs pyre/pyre-interpreter/src/importing.rsRepository: youknowone/pyre
Length of output: 5066
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '118,160p' pyre/pyre-wasm-runner/src/main.rs
printf '\n--- importing env wrapper context ---\n'
sed -n '1,70p' pyre/pyre-interpreter/src/importing.rs
python3 - <<'PY'
pairs = [(bytes([0xFF, 0xFE + i]), bytes([0])) for i in range(1, 4)]
pairs = [pair + (bytes([0x50, 0x59, 0x52, 0x45, 0x5F, 0x58]),) for pair in pairs]
for key_bytes, value_bytes in pairs:
out = []
discarded = 0
for key_bytes, value_bytes in pairs:
key = key_bytes.decode('utf-8', errors='replace'.encode()[0])
try:
out.append(key)
except UnicodeError:
discarded += 1
print(pairs)
PYRepository: youknowone/pyre
Length of output: 4680
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import os, sys
def warn_inert_current(names):
HOST_HANDLED = {"PYRE_STDLIB", "MAJIT_STATS"}
inert = []
for name in names:
try:
s = name.encode('utf-8').decode('utf-8')
inert.append(s)
except UnicodeDecodeError:
continue
inert = [
name for name in inert
if (name.startswith("PYRE_") or name.startswith("MAJIT_"))
and not (
name.startswith("PYRE_WASM_")
or name.startswith("PYRE_CHECK_")
or name in HOST_HANDLED
)
]
return sorted(set(inert))
def warn_lossy(names):
HOST_HANDLED = {"PYRE_STDLIB", "MAJIT_STATS"}
inert = []
for name in names:
s = name.encode('utf-8', errors='replace').decode('utf-8')
inert.append(s)
inert = [
name for name in inert
if (name.startswith("PYRE_") or name.startswith("MAJIT_"))
and not (
name.startswith("PYRE_WASM_")
or name.startswith("PYRE_CHECK_")
or name in HOST_HANDLED
)
]
return sorted(set(inert))
names = [
bytes([0xFF]).decode('latin-1'),
bytes([0xC3, 0xA9]).decode('latin-1'),
"PYRE_X",
"MAJIT_Y",
"PYRE_WASM_Z",
]
print("current:", warn_inert_current(names))
print("lossy:", warn_lossy(names))
print("has_non_utf8:", any(not n.isascii() for n in names))
PYRepository: youknowone/pyre
Length of output: 232
Preserve malformed environment variable names.
vars_os() returns native names, but into_string().ok() drops non-UTF-8 name strings before the PYRE_*/MAJIT_* filter, so those inert settings aren’t shown even though they are ignored by the guest. Use lossy conversion or keep OsString through filtering.
Suggested fix
- .filter_map(|(name, _)| name.into_string().ok())
+ .map(|(name, _)| name.to_string_lossy().into_owned())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut inert: Vec<String> = std::env::vars_os() | |
| .filter_map(|(name, _)| name.into_string().ok()) | |
| let mut inert: Vec<String> = std::env::vars_os() | |
| .map(|(name, _)| name.to_string_lossy().into_owned()) |
🤖 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-wasm-runner/src/main.rs` around lines 129 - 130, Update the
environment-variable collection in the inert settings initialization to preserve
non-UTF-8 names instead of dropping them via into_string().ok(). Convert names
lossily or retain OsString values through the PYRE_ and MAJIT_ filtering so
malformed names remain represented while still being ignored by the guest.
1 participant
Four commits, each independent. The headline is the last one.
interp: port the StdObjSpace finditem/finditem_str dict shortcutfinditemimplemented onlybaseobjspace.py:873's generic form — callgetitem, catchKeyError, returnNone.objspace.py:745-766overrides both it andfinditem_strfor a plain dict with what upstream calls a "performance shortcut to avoid creating the OperationError(KeyError) and allocating W_BytesObject". That override was missing here, so every dict miss built a full exception object — keyreprincluded — and threw it away.Attribute lookup takes that route on paths where a miss is the ordinary result. A guest profile of
while i < N: import mathattributes 54.9% of all samples toPyError::key_error_with_key, reached three ways, all fromgcd_import_fast:The receiver gate is upstream's
isinstance(w_obj, W_DictMultiObject) and not w_obj.user_overridden_class:is_dictanswers the user-visible isinstance question, so a module dict passes, and the exact-class comparison keeps a dict subclass — whose__getitem__or__missing__may override the probe — on the generic path.finditem's shortcut is the dict arm ofgetitem_slotminus itsdict_missing_or_key_errorbranch, which only an excluded subclass can reach, and it recovers a raising__hash__/__eq__through the sametake_pending_dict_key_error.synth/import_from_hotsynth/import_namewasm's share is larger because the discarded exception is an allocation, and allocation carries wasm's constant factor.
import_name's wasm/dynasm ratio goes 7.5x → 2.0x;import_from_hotnow runs faster on wasm than on dynasm. The three benches that looked 1.24-1.31x slower in a cross-run log comparison were a load artifact — an interleaved A/B against a baseline binary (min of 7) puts them at 0.92x / 0.96x / 0.97x.jit: check trace_limit in the single-walker loopNot an unimplemented feature — a single-walker migration regression. RPython calls
blackhole_if_trace_too_long()after everyrun_one_step()(pyjitpl.py:2865,trace_limit = 6000atrlib/jit.py:592). pyre had the machinery and called it from the legacy per-opcode loop;note_root_trace_too_longstill had a caller in the retired loop at8dfa542939b, and it did not come along tojitcode_dispatch::walk.walk()now checksis_too_long()afterstepand returns a newDispatchError::TraceTooLong(the existing_ => TraceAction::Abortfall-through picks it up). Effect onsynth/gc_iterator_source_drop(wasm): 11.80s → 4.89s,compile_ms65.0 → 9.4.&mut TraceCtxand cannot reachMetaInterp::blackhole_if_trace_too_long, so thefind_biggest_function→disable_noninlinable_functionhalf still runs only on the per-opcode path.No native bench's
loops_abortedmoved — nothing else in the suite records a >6000-op trace, so it is a pure safety net there.check: describe the jit-stats regression floor by direction, not by zeroJITSTATS_BADNESS_FIELDS's comment claimed both counters "read 0 in every healthy baseline on every platform". True ofinternal_compile_panics, not ofloops_aborted: the nine committed baselines are the only.jitstatsfixtures in the tree and all carry 1-2 aborts. Each isLoopBearingCalleeInlineUnsupported— the walker's open gap on inlining a callee whose body is not a straight-line leaf — and the counts line up 1:1 with the census underPYRE_FBW_CENSUS=1. The gate is a ratchet on the committed value, so a nonzero baseline is what it was built for; the comments now say so.wasm-runner: report PYRE_*/MAJIT_* settings the guest cannot readThe wasm guest is built for
wasm32-unknown-unknown, whosestd::envis permanently empty, soPYRE_NO_JIT,PYRE_GC_INTERP,MAJIT_STRICT,PYRE_FBW_*and friends do nothing under the wasm runner — any wasm A/B through a guest-side env var is a void experiment. The runner now names them on stderr at startup. Prefix matching with a small host-handled exemption list means a new guest knob needs no maintenance here.Side finding, left alone as out of scope:
check.py's wasm leg does not run in strict mode, becauseMAJIT_STRICTis one of the inert variables — an internal compile panic falls back to the interpreter silently rather than crashing.Verification
python3 pyre/check.pyon this branch's tip: dynasm 313/313, cranelift 313/313, wasm 310/310, no jit-stats regression. Re-run after rebasing onto7b70e1ebff9, which pulled in #785 (ImportRLock, touching the import path this PR speeds up) and #777 (fbw tracer).🤖 Generated with Claude Code
Summary by CodeRabbit