wasm backend: PR #691 review follow-ups (GuardSubclass width, IntSignext/indexed-GC decline, bridge-iter journal root) - #737
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
WalkthroughThe changes preserve wide x86 immediates, add wasm unsupported-operation handling and full-width loads, introduce a Unicode string benchmark, improve performance-gate failure details, and GC-root bridge iterator journals. ChangesDynasm immediate handling
Wasm codegen handling
String benchmark tooling
Performance gate reporting
Bridge iterator GC rooting
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 2801368). 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
|
5d5fab9 to
6aecc6e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/bench/synth/str_getitem_len_hot.py`:
- Around line 18-22: Update the loop in hot_len to use an underscore instead of
the unused i variable, changing the iteration target to for _ in range(n) while
preserving the existing accumulation behavior.
- Line 27: Update latin1_s in the test data to remove the U+2014 em dash and use
a one-byte Latin-1 character instead, then add hot_len(n, latin1_s) to the
hot_len benchmark alongside bmp_s and astral_s. Preserve the existing benchmark
structure and other inputs.
In `@pyre/check.py`:
- Around line 1137-1157: Update _gate_fail_detail so the reported ratio
preserves sufficient precision to remain distinguishable from the gate threshold
for failures just above it. Replace the one-decimal ratio formatting with higher
precision, or mark the ratio as approximate rather than asserting an exact
displayed “>” relationship; keep the existing execution-time values and fallback
handling unchanged.
🪄 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: e6768e48-e014-4108-b775-69c5c53368ae
📒 Files selected for processing (6)
majit/majit-backend-dynasm/src/x86/assembler.rsmajit/majit-backend-dynasm/tests/basic_loop.rsmajit/majit-backend-wasm/src/codegen.rspyre/bench/synth/str_getitem_len_hot.pypyre/check.pypyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
| def hot_len(n, s): | ||
| acc = 0 | ||
| for i in range(n): | ||
| acc += len(s) | ||
| return acc |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use an underscore for the intentionally unused loop variable.
hot_len does not read i; use for _ in range(n) to express the intent and clear Ruff B007.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 20-20: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
🤖 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/bench/synth/str_getitem_len_hot.py` around lines 18 - 22, Update the
loop in hot_len to use an underscore instead of the unused i variable, changing
the iteration target to for _ in range(n) while preserving the existing
accumulation behavior.
Source: Linters/SAST tools
|
|
||
| def main(): | ||
| ascii_s = "The quick brown fox jumps over the lazy dog 0123456789!?" | ||
| latin1_s = "café déjà vu naïve résumé — ¡Hola! ½¾ ©®µ" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate file =="
git ls-files | grep -F 'pyre/bench/synth/str_getitem_len_hot.py' || true
echo "== File contents =="
if [ -f pyre/bench/synth/str_getitem_len_hot.py ]; then
cat -n pyre/bench/synth/str_getitem_len_hot.py
fi
echo "== String encoding facts =="
python3 - <<'PY'
s = "café déjà vu naïve résumé — ¡Hola! ½¾ ©®µ"
print("encoding:", s.encode('latin-1').decode('latin-1') == s)
print("len_utf8_bytes:", len(s.encode('utf-8')))
print("unicode_points_high:", [(c, hex(ord(c))) for c in s if ord(c) > 0xFF])
PY
echo "== Search related benchmark helpers =="
rg -n "hot_len|hot_getitem|latin1_s|iconv|item_size|STRGETITEM|STRLEN" pyre/bench pyre -g '*.py' || trueRepository: youknowone/pyre
Length of output: 2063
Exercise the actual Latin-1 path and include it in hot_len.
latin1_s includes U+2014, so it falls back to a wider Unicode representation; replacing the em dash with a one-byte character restores the intended Latin-1 STRGETITEM input. Also call hot_len(n, latin1_s) so the STRLEN Latin-1 path is exercised alongside bmp_s and astral_s.
🤖 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/bench/synth/str_getitem_len_hot.py` at line 27, Update latin1_s in the
test data to remove the U+2014 em dash and use a one-byte Latin-1 character
instead, then add hot_len(n, latin1_s) to the hot_len benchmark alongside bmp_s
and astral_s. Preserve the existing benchmark structure and other inputs.
| def _gate_fail_detail(self, backend, baseline, measured, baseline_time, limit): | ||
| """One-line FAIL detail using the exact numbers the gate compared. | ||
|
|
||
| The gate decides on startup-subtracted exec times | ||
| (``_exec_time(backend, measured) <= _exec_time(baseline, baseline_time) | ||
| * limit``), so those exec times — not the raw run times — are printed, | ||
| alongside their true measured ratio and the gate threshold. On a FAIL | ||
| the ratio necessarily exceeds the threshold, so every number on the | ||
| line is arithmetically self-consistent: exec_measured / exec_baseline | ||
| equals the shown ratio, which is above the shown gate. | ||
| """ | ||
| exec_m = self._exec_time(backend, measured) | ||
| exec_b = self._exec_time(baseline, baseline_time) | ||
| if exec_b in (None, "-") or float(exec_b) <= 0: | ||
| ratio = "-" | ||
| else: | ||
| ratio = f"{float(exec_m) / float(exec_b):.1f}x" | ||
| return ( | ||
| f"exec {exec_m:.2f}s > {baseline} {exec_b:.2f}s " | ||
| f"ratio {ratio} > gate {float(limit):g}x" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve enough precision for the reported gate comparison.
With :.1f, a real failure just above a threshold can print identical rounded values (ratio 1.1x > gate 1.1x). Use more precision or describe the ratio as approximate instead of asserting the displayed > relation.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 1137-1137: Missing return type annotation for private function _gate_fail_detail
Add return type annotation: str
(ANN202)
🤖 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/check.py` around lines 1137 - 1157, Update _gate_fail_detail so the
reported ratio preserves sufficient precision to remain distinguishable from the
gate threshold for failures just above it. Replace the one-decimal ratio
formatting with higher precision, or mark the ratio as approximate rather than
asserting an exact displayed “>” relationship; keep the existing execution-time
values and fallback handling unchanged.
![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: 061219ee01
ℹ️ 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".
| if session.last_caught_exception_value == exc_value { | ||
| return; |
There was a problem hiding this comment.
When the same exception object was previously caught and the frame later executes an explicit raise e inside another handled block, caught is true but last_caught_exception_value still matches e; this unconditional return suppresses the new traceback entry and makes the JIT-recorded exception diverge from interpreter semantics. The old !caught guard was needed so only bare re-raises skip recording; the identical predicate in the inline-frame helper has the same issue.
AGENTS.md reference: AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/59d3cfe589f3d9ef91c2b9394d978ccc965b0004/pyre-jit/src/call_jit.rs#L706-L708
When a hot frame executes a bare raise with no active exception (for example try: raise; except RuntimeError as e: ...), the interpreter creates a new RuntimeError with attach_tb=true, so the caught exception must get a traceback node at this RAISE_VARARGS 0 line. This opcode-only predicate treats every argc-0 raise as a re-raise and record_caught_blackhole_traceback then returns before recording, so JIT/blackhole execution loses the traceback for that normal Python error; the skip needs to distinguish an actual active-exception re-raise from the freshly-created RuntimeError path.
AGENTS.md reference: AGENTS.md:L14-L18
https://github.com/youknowone/pyre/blob/59d3cfe589f3d9ef91c2b9394d978ccc965b0004/pyre-interpreter/src/module/pypyjit/mod.rs#L60-L64
For keyword calls such as pypyjit.set_param(no_such=1), this serializes the unknown name into no_such=1 and sends it through call::set_jit_param_string; that path ultimately calls WarmEnterState::set_param, whose default arm ignores unknown names, so the call silently succeeds. PyPy's interp_jit.py keyword loop raises TypeError("no JIT parameter ..."), and the old direct implementation in pyre-jit/src/eval.rs had the same is_known_jit_param check, so this new module should validate keyword names before or while applying them.
AGENTS.md reference: AGENTS.md:L194-L195
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ea2b9375d
ℹ️ 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".
| // interp_jit.py:151-156 — positional string → set_user_param(None, text). | ||
| if let Some(&text_obj) = pos.first() { | ||
| let text = crate::baseobjspace::text_w(text_obj)?; | ||
| if crate::call::set_jit_param_string(&text).is_err() { |
There was a problem hiding this comment.
When a caller misspells a parameter in the positional string, for example pypyjit.set_param("thresholdd=1"), this is_err() check never fires: the registered hook uses apply_jit_param_string, which delegates unknown names to WarmEnterState::set_param, and that method's unknown-name arm is a no-op. PyPy's set_user_param raises ValueError for unknown names, so pyre now reports success while leaving the requested JIT knob unchanged, making stress/benchmark configuration silently ineffective.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
…nd indexed GC ops - GuardSubclass read subclassrange_min at size_of::<usize>() (4 bytes on wasm32); PyType::subclassrange_min is AtomicI64, so read 8 bytes on both the vtable and gcremovetypeptr paths. - IntSignext with a non-constant num_bytes operand aborted via panic; return BackendError::Unsupported so the trace declines to interpreter fallback. - gc_load_indexed_*/gc_store_indexed_* are frontend blackhole ops that can reach the backend; return Unsupported for the indexed forms instead of panicking. The bare GcLoad*/GcStore GC-rewrite forms keep the panic. Assisted-by: Claude
FBW_BRIDGE_ITER_JOURNAL stores range-iterator refs across an authoritative bridge walk but was visited by no root walker, unlike the five sibling FBW journals. Add it to FbwStoreJournalRootArea and fbw_store_journal_root_walker_ area so a minor collection forwards the iterator before the non-commit rollback restores its cursor via w_range_iter_set_cursor. Assisted-by: Claude
The FAIL line printed raw run times and the gate threshold formatted as if it were the measured ratio, so the numbers were not self-consistent. Add _gate_fail_detail to print the startup-subtracted exec times the gate actually compared, their ratio, and the threshold. Assisted-by: Claude
…decline as verified inert - bench/synth/str_getitem_len_hot.py: hot str/unicode subscript and len over ASCII/latin1/BMP/astral strings (item_size 1/2/4), routed through the GETARRAYITEM/ARRAYLEN paths; output asserted cpython==pypy. - codegen.rs: a str-subscript / len / compare / find hot loop traces to GETARRAYITEM, never STRGETITEM (verified with PYRE_DUMP_PERFN_JITCODE), so the STRGETITEM/UNICODEGETITEM/STRLEN/UNICODELEN decline covers ops no trace emits; note this so the decline is not mistaken for a missing descr-driven lowering. Assisted-by: Claude
emit_binop_reg_loc's Loc::Immed arm truncated the value with `as i32`, encoding an out-of-i32-range immediate as a sign-extended imm32 — `x & 0xFFFF_FFFF_FFFF` degenerated to `x & -1` in compiled code, the wrong output of synth/str_getitem_len_hot on dynasm. Follow regloc.py:456-464: mov the value into X86_64_SCRATCH_REG and retry the reg-reg form. The IntAdd LEA emitter gets the same fallback for its immediate arm, which the consider_binop_symm path reaches with an arbitrary 64-bit constant; its two symmetric arms are merged. Two backend tests compile and execute AND/ADD with wide immediates.
Register a pypyjit module whose set_param accepts the positional-string
form ("name=value,…", "off", "default") and keyword arguments, routing
both through the JIT's set_user_param parser. pyre-interpreter cannot
import pyre-jit, so add a SET_JIT_PARAM_STRING_HOOK alongside the existing
per-pair SET_JIT_PARAM_HOOK; pyre-jit registers
set_jit_param_string_via_warmstate at boot and per-eval. The hook is an
in-process function pointer, so a pypyjit.set_param call configures the
warmstate on every backend including the wasm guest, which sees no
environment.
…nches
exception_metadata_jitstress and exception_reraise_tb_depth_jitstress call
pypyjit.set_param("threshold=1,function_threshold=1") so trace recording
fires on the earliest iterations of every section rather than after the
~1600-iteration warmup. Recording then lands on the traceback/context/
exc_info/reraise shapes on every run and every backend, making coverage of
the recording path deterministic instead of dependent on which iteration a
warmup pass happens to hit. The import is guarded so the benches run
unchanged under CPython, which has no pypyjit. Output matches the
natural-threshold twins.
A module-level hot loop executes bare re-raise (depth 2), named re-raise (depth 3), and finally-passthrough (depth 2) so the recording iteration itself runs the re-raise chain. Guards the instruction-keyed traceback recording against spurious nodes at re-raise / handler-cleanup coordinates.
1 participant
Follow-up fixes from the Codex/CodeRabbit review of the (now-merged) #691, verified against current
main.Commits
majit-backend-wasm/src/codegen.rsfixes:GuardSubclassreadsubclassrange_minatsize_of::<usize>()(4 bytes on wasm32);PyType::subclassrange_minisAtomicI64, so read 8 bytes on both the vtable and gcremovetypeptr paths.IntSignextwith a non-constantnum_bytesoperand aborted viapanic!; a non-constant width is a valid IR shape (the cranelift backend resolvesarg(1)at runtime), so returnBackendError::Unsupportedfor interpreter fallback.gc_load_indexed_*/gc_store_indexed_*are real frontend blackhole ops that can reach the backend; decline the indexed forms withUnsupportedinstead of panicking. The bareGcLoad*/GcStoreGC-rewrite forms keep the loud panic.FBW_BRIDGE_ITER_JOURNALheld range-iterator refs across an authoritative bridge walk but was visited by no root walker, unlike its five sibling FBW journals. Wire it intoFbwStoreJournalRootArea/fbw_store_journal_root_walker_areaso a minor collection forwards the iterator before the non-commit rollback restores its cursor.Review findings not acted on
inline_subwalk: true, sotry_walker_specialize_for_iter_nextdeclines before the journal push; the journal is never populated in a subwalk.Verification
python3 pyre/check.py --backend wasm→ 290/290.python3 pyre/check.py --backend dynasm→ 293/293. Both green, 0 failures.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Performance